| Message ID | 1a25b4f079dcdc669d4b29d3658ef0b72be2651e.1587742492.git-series.maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: Support BCM2711 Display Pipeline | expand |
Hi Maxime, as always, thanks for the series! Some extra context, and comments below. On Fri, 2020-04-24 at 17:34 +0200, Maxime Ripard wrote: > The RaspberryPi4 firmware actually exposes more clocks than are currently > handled by the driver and we will need to change some of them directly > based on the pixel rate for the display related clocks, or the load for the > GPU. > > This rate change can have a number of side-effects, including adjusting the > various PLL voltages or the PLL parents. The firmware will also update > those clocks by itself for example if the SoC runs too hot. To complete this: RPi4's firmware implements DVFS. Clock frequency and SoC voltage are correlated, if you can determine all clocks are running below a maximum then it should be safe to lower the voltage. Currently, firmware actively monitors arm, core, h264, v3d, isp and hevc to determine what voltage can be reduced to, and if arm increases any of those clocks behind the firmware's back, when it has a lowered voltage, a crash is highly likely. As pointed out to me by RPi foundation engineers hsm/pixel aren't currently being monitored, as driving a high resolution display also requires a high core clock, which already sets an acceptable min voltage threshold. But that might change if needed. Ultimately, from the DVFS, the safe way to change clocks from arm would be to always use the firmware interface, which we're far from doing right now. On the other hand, AFAIK not all clocks have a firmware counterpart. Note that we could also have a word with the RPi foundation and see if disabling DVFS is possible (AFAIK it's not an option right now). Although I personally prefer to support this upstream, aside from the obvious benefits to battery powered use cases, not consuming power unnecessarily is always big plus. > In order to make Linux play as nice as possible with those constraints, it > makes sense to rely on the firmware clocks as much as possible. As I comment above, not as simple as it seems. I suggest, for now, we only register the clocks we're going to use and that are likely to be affected by DVFS. hsm being a contender here. As we'd be settling on a hybrid solution here, which isn't ideal to say the least, I'd like to gather some opinions on whether pushing towards a fully firmware based solution is something you'd like to see. > Fortunately,t he firmware has an interface to discover the clocks it > exposes. nit: wrongly placed space > Let's use it to discover, register the clocks in the clocks framework and > then expose them through the device tree for consumers to use them. > > 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 | 104 +++++++++++++++++++--- [...] > +static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, > + unsigned int parent, > + unsigned int id) > +{ > + struct raspberrypi_clk_data *data; > + struct clk_init_data init = {}; > + int ret; > + > + if (id == RPI_FIRMWARE_ARM_CLK_ID) { > + struct clk_hw *hw; > + > + hw = raspberrypi_register_pllb(rpi); > + if (IS_ERR(hw)) { > + dev_err(rpi->dev, "Failed to initialize pllb, %ld\n", > + PTR_ERR(hw)); > + return hw; > + } > + > + return raspberrypi_register_pllb_arm(rpi); > + } We shouldn't create a special case for pllb. My suggestion here is that we revert the commit that removed pllb from the mmio driver, in order to maintain a nice view of the clock tree, and register this as a regular fw-clk. The only user to this is RPi's the cpufreq driver, which shouldn't mind the change as long as you maintain the clk lookup creation. On that topic, now that the clocks are available in DT we could even move raspberrypi-cpufreq's registration there. But that's out of the scope of this series. > + > + data = devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return ERR_PTR(-ENOMEM); > + data->rpi = rpi; > + data->id = id; > + > + init.name = devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id); I'd really like more descriptive names here, sadly the firmware interface doesn't provide them, but they are set in stone nonetheless. Something like 'fw-clk-arm' and 'fw-clk-hsm' comes to mind (SCMI does provide naming BTW). See here for a list of all clocks: https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#clocks > + init.ops = &raspberrypi_firmware_clk_ops; > + init.flags = CLK_GET_RATE_NOCACHE; > + > + data->hw.init = &init; > + > + ret = devm_clk_hw_register(rpi->dev, &data->hw); > + if (ret) > + return ERR_PTR(ret); > + > + return &data->hw; > +} > + > +static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi, > + struct clk_hw_onecell_data *data) > +{ > + struct rpi_firmware_get_clocks_response *clks; > + int ret; > + > + clks = devm_kcalloc(rpi->dev, sizeof(*clks), NUM_FW_CLKS, GFP_KERNEL); > + if (!clks) > + return -ENOMEM; > + > + ret = rpi_firmware_property(rpi->firmware, RPI_FIRMWARE_GET_CLOCKS, > + clks, sizeof(*clks) * NUM_FW_CLKS); > + if (ret) > + return ret; > + > + while (clks->id) { > + struct clk_hw *hw; Se my comment above WRT registering all clocks. Regards, Nicolas
Hi Nicolas, On Mon, May 04, 2020 at 02:05:47PM +0200, Nicolas Saenz Julienne wrote: > Hi Maxime, as always, thanks for the series! > Some extra context, and comments below. > > On Fri, 2020-04-24 at 17:34 +0200, Maxime Ripard wrote: > > The RaspberryPi4 firmware actually exposes more clocks than are currently > > handled by the driver and we will need to change some of them directly > > based on the pixel rate for the display related clocks, or the load for the > > GPU. > > > > This rate change can have a number of side-effects, including adjusting the > > various PLL voltages or the PLL parents. The firmware will also update > > those clocks by itself for example if the SoC runs too hot. > > To complete this: > > RPi4's firmware implements DVFS. Clock frequency and SoC voltage are > correlated, if you can determine all clocks are running below a maximum then it > should be safe to lower the voltage. Currently, firmware actively monitors arm, > core, h264, v3d, isp and hevc to determine what voltage can be reduced to, and > if arm increases any of those clocks behind the firmware's back, when it has a > lowered voltage, a crash is highly likely. > > As pointed out to me by RPi foundation engineers hsm/pixel aren't currently > being monitored, as driving a high resolution display also requires a high core > clock, which already sets an acceptable min voltage threshold. But that might > change if needed. > > Ultimately, from the DVFS, the safe way to change clocks from arm would be to > always use the firmware interface, which we're far from doing right now. On the > other hand, AFAIK not all clocks have a firmware counterpart. > > Note that we could also have a word with the RPi foundation and see if > disabling DVFS is possible (AFAIK it's not an option right now). Although I > personally prefer to support this upstream, aside from the obvious benefits to > battery powered use cases, not consuming power unnecessarily is always big > plus. > > > In order to make Linux play as nice as possible with those constraints, it > > makes sense to rely on the firmware clocks as much as possible. > > As I comment above, not as simple as it seems. I suggest, for now, we only > register the clocks we're going to use and that are likely to be affected by > DVFS. hsm being a contender here. > > As we'd be settling on a hybrid solution here, which isn't ideal to say the > least, I'd like to gather some opinions on whether pushing towards a fully > firmware based solution is something you'd like to see. Thanks for the summary above, I'll try to adjust that commit log to reflect this. As for my opinion, I don't really think that an hybrid approach is practical, since that would leave us with weird interactions between the firmware (and possibly multiple versinos of it) and the linux driver, and this would be pretty hard to maintain in the long run. That leaves us either the MMIO-based driver or the firmware-based one, and here with what you said above, at the moment, the firmware based one is a clear winner. > > Fortunately,t he firmware has an interface to discover the clocks it > > exposes. > > nit: wrongly placed space > > > Let's use it to discover, register the clocks in the clocks framework and > > then expose them through the device tree for consumers to use them. > > > > 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 | 104 +++++++++++++++++++--- > > [...] > > > +static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, > > + unsigned int parent, > > + unsigned int id) > > +{ > > + struct raspberrypi_clk_data *data; > > + struct clk_init_data init = {}; > > + int ret; > > + > > + if (id == RPI_FIRMWARE_ARM_CLK_ID) { > > + struct clk_hw *hw; > > + > > + hw = raspberrypi_register_pllb(rpi); > > + if (IS_ERR(hw)) { > > + dev_err(rpi->dev, "Failed to initialize pllb, %ld\n", > > + PTR_ERR(hw)); > > + return hw; > > + } > > + > > + return raspberrypi_register_pllb_arm(rpi); > > + } > > We shouldn't create a special case for pllb. My suggestion here is that we > revert the commit that removed pllb from the mmio driver, in order to maintain > a nice view of the clock tree, and register this as a regular fw-clk. > > The only user to this is RPi's the cpufreq driver, which shouldn't mind the > change as long as you maintain the clk lookup creation. Ok, I'll change that. > On that topic, now that the clocks are available in DT we could even move > raspberrypi-cpufreq's registration there. But that's out of the scope of this > series. > > > + > > + data = devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return ERR_PTR(-ENOMEM); > > + data->rpi = rpi; > > + data->id = id; > > + > > + init.name = devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id); > > I'd really like more descriptive names here, sadly the firmware interface > doesn't provide them, but they are set in stone nonetheless. Something like > 'fw-clk-arm' and 'fw-clk-hsm' comes to mind (SCMI does provide naming BTW). > > See here for a list of all clocks: > https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#clocks That's a good idea, I'll add that too. Thanks! Maxime
Hi Maxime, On Fri, 2020-05-15 at 10:19 +0200, Maxime Ripard wrote: > Hi Nicolas, > > On Mon, May 04, 2020 at 02:05:47PM +0200, Nicolas Saenz Julienne wrote: > > Hi Maxime, as always, thanks for the series! > > Some extra context, and comments below. > > > > On Fri, 2020-04-24 at 17:34 +0200, Maxime Ripard wrote: > > > The RaspberryPi4 firmware actually exposes more clocks than are currently > > > handled by the driver and we will need to change some of them directly > > > based on the pixel rate for the display related clocks, or the load for > > > the > > > GPU. > > > > > > This rate change can have a number of side-effects, including adjusting > > > the > > > various PLL voltages or the PLL parents. The firmware will also update > > > those clocks by itself for example if the SoC runs too hot. > > > > To complete this: > > > > RPi4's firmware implements DVFS. Clock frequency and SoC voltage are > > correlated, if you can determine all clocks are running below a maximum then > > it > > should be safe to lower the voltage. Currently, firmware actively monitors > > arm, > > core, h264, v3d, isp and hevc to determine what voltage can be reduced to, > > and > > if arm increases any of those clocks behind the firmware's back, when it has > > a > > lowered voltage, a crash is highly likely. > > > > As pointed out to me by RPi foundation engineers hsm/pixel aren't currently > > being monitored, as driving a high resolution display also requires a high > > core > > clock, which already sets an acceptable min voltage threshold. But that > > might > > change if needed. > > > > Ultimately, from the DVFS, the safe way to change clocks from arm would be > > to > > always use the firmware interface, which we're far from doing right now. On > > the > > other hand, AFAIK not all clocks have a firmware counterpart. > > > > Note that we could also have a word with the RPi foundation and see if > > disabling DVFS is possible (AFAIK it's not an option right now). Although I > > personally prefer to support this upstream, aside from the obvious benefits > > to > > battery powered use cases, not consuming power unnecessarily is always big > > plus. > > > > > In order to make Linux play as nice as possible with those constraints, it > > > makes sense to rely on the firmware clocks as much as possible. > > > > As I comment above, not as simple as it seems. I suggest, for now, we only > > register the clocks we're going to use and that are likely to be affected by > > DVFS. hsm being a contender here. > > > > As we'd be settling on a hybrid solution here, which isn't ideal to say the > > least, I'd like to gather some opinions on whether pushing towards a fully > > firmware based solution is something you'd like to see. > > Thanks for the summary above, I'll try to adjust that commit log to reflect > this. As for my opinion, I don't really think that an hybrid approach is > practical, since that would leave us with weird interactions between the > firmware (and possibly multiple versinos of it) and the linux driver, and this > would be pretty hard to maintain in the long run. > > That leaves us either the MMIO-based driver or the firmware-based one, and > here > with what you said above, at the moment, the firmware based one is a clear > winner. We're on the same page here :) My remaining concern is the fact there isn't a firmware counterpart to every clock used right now. But it's something we can work out in the future. Regards, Nicolas
Quoting Maxime Ripard (2020-04-24 08:34:01) > The RaspberryPi4 firmware actually exposes more clocks than are currently > handled by the driver and we will need to change some of them directly > based on the pixel rate for the display related clocks, or the load for the > GPU. > > This rate change can have a number of side-effects, including adjusting the > various PLL voltages or the PLL parents. The firmware will also update > those clocks by itself for example if the SoC runs too hot. > > In order to make Linux play as nice as possible with those constraints, it > makes sense to rely on the firmware clocks as much as possible. > > Fortunately,t he firmware has an interface to discover the clocks it Fortunately, the > exposes. > > Let's use it to discover, register the clocks in the clocks framework and > then expose them through the device tree for consumers to use them. > > 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 1a9027169a2a..6a789749aea6 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -285,6 +285,95 @@ static struct clk_hw *raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi) return &raspberrypi_clk_pllb_arm.hw; } +static long raspberrypi_fw_dumb_round_rate(struct clk_hw *hw, + unsigned long rate, + unsigned long *parent_rate) +{ + /* + * The firmware will do the rounding but that isn't part of + * the interface with the firmware, so we just do our best + * here. + */ + return rate; +} + +static const struct clk_ops raspberrypi_firmware_clk_ops = { + .is_prepared = raspberrypi_fw_is_prepared, + .recalc_rate = raspberrypi_fw_get_rate, + .round_rate = raspberrypi_fw_dumb_round_rate, + .set_rate = raspberrypi_fw_set_rate, +}; + +static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, + unsigned int parent, + unsigned int id) +{ + struct raspberrypi_clk_data *data; + struct clk_init_data init = {}; + int ret; + + if (id == RPI_FIRMWARE_ARM_CLK_ID) { + struct clk_hw *hw; + + hw = raspberrypi_register_pllb(rpi); + if (IS_ERR(hw)) { + dev_err(rpi->dev, "Failed to initialize pllb, %ld\n", + PTR_ERR(hw)); + return hw; + } + + return raspberrypi_register_pllb_arm(rpi); + } + + data = devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return ERR_PTR(-ENOMEM); + data->rpi = rpi; + data->id = id; + + init.name = devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id); + init.ops = &raspberrypi_firmware_clk_ops; + init.flags = CLK_GET_RATE_NOCACHE; + + data->hw.init = &init; + + ret = devm_clk_hw_register(rpi->dev, &data->hw); + if (ret) + return ERR_PTR(ret); + + return &data->hw; +} + +static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi, + struct clk_hw_onecell_data *data) +{ + struct rpi_firmware_get_clocks_response *clks; + int ret; + + clks = devm_kcalloc(rpi->dev, sizeof(*clks), NUM_FW_CLKS, GFP_KERNEL); + if (!clks) + return -ENOMEM; + + ret = rpi_firmware_property(rpi->firmware, RPI_FIRMWARE_GET_CLOCKS, + clks, sizeof(*clks) * NUM_FW_CLKS); + if (ret) + return ret; + + while (clks->id) { + struct clk_hw *hw; + + hw = raspberrypi_clk_register(rpi, clks->parent, clks->id); + if (IS_ERR(hw)) + return PTR_ERR(hw); + + data->hws[clks->id] = hw; + data->num = clks->id + 1; + clks++; + } + + return 0; +} + static int raspberrypi_clk_probe(struct platform_device *pdev) { struct clk_hw_onecell_data *clk_data; @@ -292,7 +381,6 @@ static int raspberrypi_clk_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct rpi_firmware *firmware; struct raspberrypi_clk *rpi; - struct clk_hw *hw; int ret; /* @@ -327,17 +415,9 @@ static int raspberrypi_clk_probe(struct platform_device *pdev) if (!clk_data) return -ENOMEM; - hw = raspberrypi_register_pllb(rpi); - if (IS_ERR(hw)) { - dev_err(dev, "Failed to initialize pllb, %ld\n", PTR_ERR(hw)); - return PTR_ERR(hw); - } - - hw = raspberrypi_register_pllb_arm(rpi); - if (IS_ERR(hw)) - return PTR_ERR(hw); - clk_data->hws[RPI_FIRMWARE_ARM_CLK_ID] = hw; - clk_data->num = RPI_FIRMWARE_ARM_CLK_ID + 1; + ret = raspberrypi_discover_clocks(rpi, clk_data); + if (ret) + return ret; ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h index 7800e12ee042..27bfc0dcfa9b 100644 --- a/include/soc/bcm2835/raspberrypi-firmware.h +++ b/include/soc/bcm2835/raspberrypi-firmware.h @@ -135,6 +135,11 @@ enum rpi_firmware_property_tag { RPI_FIRMWARE_GET_DMA_CHANNELS = 0x00060001, }; +struct rpi_firmware_get_clocks_response { + __le32 parent; + __le32 id; +}; + #if IS_ENABLED(CONFIG_RASPBERRYPI_FIRMWARE) int rpi_firmware_property(struct rpi_firmware *fw, u32 tag, void *data, size_t len);
The RaspberryPi4 firmware actually exposes more clocks than are currently handled by the driver and we will need to change some of them directly based on the pixel rate for the display related clocks, or the load for the GPU. This rate change can have a number of side-effects, including adjusting the various PLL voltages or the PLL parents. The firmware will also update those clocks by itself for example if the SoC runs too hot. In order to make Linux play as nice as possible with those constraints, it makes sense to rely on the firmware clocks as much as possible. Fortunately,t he firmware has an interface to discover the clocks it exposes. Let's use it to discover, register the clocks in the clocks framework and then expose them through the device tree for consumers to use them. 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 | 104 +++++++++++++++++++--- include/soc/bcm2835/raspberrypi-firmware.h | 5 +- 2 files changed, 97 insertions(+), 12 deletions(-)