| Message ID | 20240122-gdsc-hwctrl-v4-0-9061e8a7aa07@linaro.org |
|---|---|
| Headers | show |
| Series | PM: domains: Add control for switching back and forth to HW control | expand |
[...] > + > +/** > + * dev_pm_genpd_get_hwmode - Get the HW mode setting for the device. > + * > + * @dev: Device for which the current HW-mode setting should be fetched. > + * > + * This helper function allows consumer drivers to fetch the current HW mode > + * setting of its the device. > + * > + * It is assumed that the users guarantee that the genpd wouldn't be detached > + * while this routine is getting called. > + */ > +bool dev_pm_genpd_get_hwmode(struct device *dev) > +{ > + struct generic_pm_domain *genpd; > + > + genpd = dev_to_genpd_safe(dev); > + if (!genpd) > + return false; > + > + if (genpd->get_hwmode_dev) > + return genpd->get_hwmode_dev(genpd, dev); Not sure why I haven't spotted this before - but we should probably assign dev_gpd_data(dev)->hw_mode here, rather than returning the result from the callback directly. > + > + return dev_gpd_data(dev)->hw_mode; > +} > +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_hwmode); [...] Kind regards Uffe
On Mon, 22 Jan 2024 at 09:47, Abel Vesa <abel.vesa@linaro.org> wrote: > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > Changes in v4: > - Re-worded 1st patch commit message, as per Bjorn's suggestion, and added > Dmitry's R-b tag > - Added Bjorn's and Dmitry's R-b tags to the 2nd patch > - Re-worded 3rd patch commit message, to better explain the HW_CTRL_TRIGGER flag. > - Added mode transition delay when setting mode for GDSC > - Added status polling if GDSSC is enabled when transitioning from HW to SW > - Re-worded 4th patch commit message to better explain why the > HW_CTRL_TRIGGER needs to be used instead > - Drop changes to SC7180, SDM845 and SM8550 video CC drivers, as only > SC7280 and SM8250 have been tested so far. More platforms (with v6 venus) > will be added eventually. > - Call genpd set_hwmode API only for v6 and dropped the vcodec_pmdomains_hwctrl. > - Re-worded 5th patch commit message accordingly. > - Link to v3: > https://lore.kernel.org/r/20231101-gdsc-hwctrl-v3-0-0740ae6b2b04@linaro.org/ > > --- > Abel Vesa (1): > PM: domains: Add the domain HW-managed mode to the summary > > Jagadeesh Kona (3): > clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode > clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode > venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode > > Ulf Hansson (1): > PM: domains: Allow devices attached to genpd to be managed by HW > > drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++ > drivers/clk/qcom/gdsc.h | 1 + > drivers/clk/qcom/videocc-sc7280.c | 2 +- > drivers/clk/qcom/videocc-sm8250.c | 4 +- > drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++---- > drivers/pmdomain/core.c | 83 +++++++++++++++++++++++++- > include/linux/pm_domain.h | 17 ++++++ > 7 files changed, 169 insertions(+), 15 deletions(-) > --- Bjorn, if it helps, I can funnel this complete series via my pmdomain tree? Another option is that I host an immutable branch with patch1 and patch2 for you to pull in? Just let me know what you prefer. Kind regards Uffe
On Mon, Jan 22, 2024 at 10:47:03AM +0200, Abel Vesa wrote: > From: Jagadeesh Kona <quic_jkona@quicinc.com> > > Add support for set and get hwmode callbacks to switch the GDSC between > SW and HW modes. Currently, the GDSC is moved to HW control mode > using HW_CTRL flag and if this flag is present, GDSC is moved to HW > mode as part of GDSC enable itself. The intention is to keep the > HW_CTRL flag functionality as is, since many older chipsets still use > this flag. > This provides insight into why we end up with both HW_CTRL and HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but rather just an implementation detail. > But consumer drivers also require the GDSC mode to be switched dynamically > at runtime based on requirement for certain usecases. Some of these > usecases are switching the GDSC to SW mode to keep it ON during the > enablement of clocks that are dependent on GDSC and while programming > certain configurations that require GDSC to be ON. Introduce a new > HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev > callbacks which allows the consumer drivers to switch the GDSC back and > forth between HW/SW modes dynamically at runtime using new > dev_pm_genpd_set_hwmode API. > This still expresses the need for HW_CTRL_TRIGGER in terms of "some drivers need for some use case". We don't need these many words to say: "Introduce HW_CTRL_TRIGGER for client drivers that need it." I find that it would be useful to document that every time a GDSC is turned on the mode will be switched to SW... > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/qcom/gdsc.h | 1 + > 2 files changed, 55 insertions(+) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index 5358e28122ab..71626eb20101 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -363,6 +363,56 @@ static int gdsc_disable(struct generic_pm_domain *domain) > return 0; > } > > +static int gdsc_set_hwmode(struct generic_pm_domain *domain, struct device *dev, bool mode) > +{ > + struct gdsc *sc = domain_to_gdsc(domain); > + u32 val; > + int ret; > + > + if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) { Why is this a restriction only for GDSCs supplied by regulators? I don't find anything preventing this API from being called on GDSCs supplied by other genpd instances. Also note that regulator_is_enabled() is racy, in that it tells us if the regulator is currently turned on, not if we're the one holding that vote. As such this might change at any moment - and hence shouldn't be significant here. > + pr_err("Cannot set mode while parent is disabled\n"); > + return -EIO; > + } > + > + ret = gdsc_hwctrl(sc, mode); > + if (ret) > + return ret; > + > + /* Wait for 1usec for mode transition to properly complete */ > + udelay(1); > + > + if (!mode) { > + ret = regmap_read(sc->regmap, sc->gdscr, &val); > + if (ret) > + return ret; > + > + /* > + * While switching from HW to SW mode, if GDSC is in enabled > + * state, poll for GDSC to complete the power up. > + */ I had to give this some thought, to conclude that this is relevant if HW has the GDSC disabled and we're switching to SW - which would then enable it. I think this comment can be improved slightly, to save the reader the need for figuring out this on their own. > + if (!(val & SW_COLLAPSE_MASK)) This not being true, would imply that gdsc_disable() has been called already, in which case there's no guarantee that the parent still supplies power. In the introduced API power on and hw control are orthogonal states, but not so in this implementation. This need to made clear, to reduce future surprises. > + return gdsc_poll_status(sc, GDSC_ON); > + } > + > + return 0; > +} > + > +static bool gdsc_get_hwmode(struct generic_pm_domain *domain, struct device *dev) > +{ > + struct gdsc *sc = domain_to_gdsc(domain); > + u32 val; > + int ret; > + > + ret = regmap_read(sc->regmap, sc->gdscr, &val); > + if (ret) > + return ret; > + > + if (val & HW_CONTROL_MASK) > + return true; > + > + return false; return !!(val & HW_CONTROL_MASK); Regards, Bjorn > +} > + > static int gdsc_init(struct gdsc *sc) > { > u32 mask, val; > @@ -451,6 +501,10 @@ static int gdsc_init(struct gdsc *sc) > sc->pd.power_off = gdsc_disable; > if (!sc->pd.power_on) > sc->pd.power_on = gdsc_enable; > + if (sc->flags & HW_CTRL_TRIGGER) { > + sc->pd.set_hwmode_dev = gdsc_set_hwmode; > + sc->pd.get_hwmode_dev = gdsc_get_hwmode; > + } > > ret = pm_genpd_init(&sc->pd, NULL, !on); > if (ret) > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h > index 803512688336..1e2779b823d1 100644 > --- a/drivers/clk/qcom/gdsc.h > +++ b/drivers/clk/qcom/gdsc.h > @@ -67,6 +67,7 @@ struct gdsc { > #define ALWAYS_ON BIT(6) > #define RETAIN_FF_ENABLE BIT(7) > #define NO_RET_PERIPH BIT(8) > +#define HW_CTRL_TRIGGER BIT(9) > struct reset_controller_dev *rcdev; > unsigned int *resets; > unsigned int reset_count; > > -- > 2.34.1 >
On Tue, Jan 30, 2024 at 05:00:28PM -0600, Bjorn Andersson wrote: > On Mon, Jan 22, 2024 at 10:47:03AM +0200, Abel Vesa wrote: > > From: Jagadeesh Kona <quic_jkona@quicinc.com> > > > > Add support for set and get hwmode callbacks to switch the GDSC between > > SW and HW modes. Currently, the GDSC is moved to HW control mode > > using HW_CTRL flag and if this flag is present, GDSC is moved to HW > > mode as part of GDSC enable itself. The intention is to keep the > > HW_CTRL flag functionality as is, since many older chipsets still use > > this flag. > > > > This provides insight into why we end up with both HW_CTRL and > HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but > rather just an implementation detail. > > > But consumer drivers also require the GDSC mode to be switched dynamically > > at runtime based on requirement for certain usecases. Some of these > > usecases are switching the GDSC to SW mode to keep it ON during the > > enablement of clocks that are dependent on GDSC and while programming > > certain configurations that require GDSC to be ON. Introduce a new > > HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev > > callbacks which allows the consumer drivers to switch the GDSC back and > > forth between HW/SW modes dynamically at runtime using new > > dev_pm_genpd_set_hwmode API. > > > > This still expresses the need for HW_CTRL_TRIGGER in terms of "some > drivers need for some use case". We don't need these many words to say: > "Introduce HW_CTRL_TRIGGER for client drivers that need it." > > > I find that it would be useful to document that every time a GDSC is > turned on the mode will be switched to SW... > I believe I'm wrong here. Reading the patch again, I think we might retain the mode across a disable/enable cycle. I at least don't see anything explicit returning us to SW mode. According to Linux though, the GDSC is off, so as described below, there will be no votes for supplying resources. Regards, Bjorn
On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: > From: Ulf Hansson <ulf.hansson@linaro.org> > > Some power-domains may be capable of relying on the HW to control the power > for a device that's hooked up to it. Typically, for these kinds of > configurations the consumer driver should be able to change the behavior of > power domain at runtime, control the power domain in SW mode for certain > configurations and handover the control to HW mode for other usecases. > > To allow a consumer driver to change the behaviour of the PM domain for its > device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, > let's add a corresponding optional genpd callback, ->set_hwmode_dev(), > which the genpd provider should implement if it can support switching > between HW controlled mode and SW controlled mode. Similarly, add the > dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and > its corresponding optional genpd callback, ->get_hwmode_dev(), which the > genpd provider can also implement for reading back the mode from the > hardware. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm_domain.h | 17 ++++++++++++ > 2 files changed, 86 insertions(+) > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > index a1f6cba3ae6c..41b6411d0ef5 100644 > --- a/drivers/pmdomain/core.c > +++ b/drivers/pmdomain/core.c > @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) > } > EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); > > +/** > + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. This isn't proper kernel-doc > + * > + * @dev: Device for which the HW-mode should be changed. > + * @enable: Value to set or unset the HW-mode. > + * > + * Some PM domains can rely on HW signals to control the power for a device. To > + * allow a consumer driver to switch the behaviour for its device in runtime, > + * which may be beneficial from a latency or energy point of view, this function > + * may be called. > + * > + * It is assumed that the users guarantee that the genpd wouldn't be detached > + * while this routine is getting called. > + * > + * Returns 0 on success and negative error values on failures. > + */ > +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) > +{ > + struct generic_pm_domain *genpd; > + int ret = 0; > + > + genpd = dev_to_genpd_safe(dev); > + if (!genpd) > + return -ENODEV; > + > + if (!genpd->set_hwmode_dev) > + return -EOPNOTSUPP; > + > + genpd_lock(genpd); > + > + if (dev_gpd_data(dev)->hw_mode == enable) Between this and the gdsc patch, the hw_mode state might not match the hardware state at boot. With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, false) will not bring control to SW - which might be fatal. Regards, Bjorn
On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: > > On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > > Some power-domains may be capable of relying on the HW to control the power > > for a device that's hooked up to it. Typically, for these kinds of > > configurations the consumer driver should be able to change the behavior of > > power domain at runtime, control the power domain in SW mode for certain > > configurations and handover the control to HW mode for other usecases. > > > > To allow a consumer driver to change the behaviour of the PM domain for its > > device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, > > let's add a corresponding optional genpd callback, ->set_hwmode_dev(), > > which the genpd provider should implement if it can support switching > > between HW controlled mode and SW controlled mode. Similarly, add the > > dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and > > its corresponding optional genpd callback, ->get_hwmode_dev(), which the > > genpd provider can also implement for reading back the mode from the > > hardware. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/pm_domain.h | 17 ++++++++++++ > > 2 files changed, 86 insertions(+) > > > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > > index a1f6cba3ae6c..41b6411d0ef5 100644 > > --- a/drivers/pmdomain/core.c > > +++ b/drivers/pmdomain/core.c > > @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) > > } > > EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); > > > > +/** > > + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. > > This isn't proper kernel-doc Sorry, I didn't quite get that. What is wrong? > > > + * > > + * @dev: Device for which the HW-mode should be changed. > > + * @enable: Value to set or unset the HW-mode. > > + * > > + * Some PM domains can rely on HW signals to control the power for a device. To > > + * allow a consumer driver to switch the behaviour for its device in runtime, > > + * which may be beneficial from a latency or energy point of view, this function > > + * may be called. > > + * > > + * It is assumed that the users guarantee that the genpd wouldn't be detached > > + * while this routine is getting called. > > + * > > + * Returns 0 on success and negative error values on failures. > > + */ > > +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) > > +{ > > + struct generic_pm_domain *genpd; > > + int ret = 0; > > + > > + genpd = dev_to_genpd_safe(dev); > > + if (!genpd) > > + return -ENODEV; > > + > > + if (!genpd->set_hwmode_dev) > > + return -EOPNOTSUPP; > > + > > + genpd_lock(genpd); > > + > > + if (dev_gpd_data(dev)->hw_mode == enable) > > Between this and the gdsc patch, the hw_mode state might not match the > hardware state at boot. > > With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, > false) will not bring control to SW - which might be fatal. Right, good point. I think we have two ways to deal with this: 1) If the provider is supporting ->get_hwmode_dev(), we can let genpd_add_device() invoke it to synchronize the state. 2) If the provider doesn't support ->get_hwmode_dev() we need to call ->set_hwmode_dev() to allow an initial state to be set. The question is then, if we need to allow ->get_hwmode_dev() to be optional, if the ->set_hwmode_dev() is supported - or if we can require it. What's your thoughts around this? Kind regards Uffe
On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: > On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: > > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > Some power-domains may be capable of relying on the HW to control the power > > > for a device that's hooked up to it. Typically, for these kinds of > > > configurations the consumer driver should be able to change the behavior of > > > power domain at runtime, control the power domain in SW mode for certain > > > configurations and handover the control to HW mode for other usecases. > > > > > > To allow a consumer driver to change the behaviour of the PM domain for its > > > device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, > > > let's add a corresponding optional genpd callback, ->set_hwmode_dev(), > > > which the genpd provider should implement if it can support switching > > > between HW controlled mode and SW controlled mode. Similarly, add the > > > dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and > > > its corresponding optional genpd callback, ->get_hwmode_dev(), which the > > > genpd provider can also implement for reading back the mode from the > > > hardware. > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/pm_domain.h | 17 ++++++++++++ > > > 2 files changed, 86 insertions(+) > > > > > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > > > index a1f6cba3ae6c..41b6411d0ef5 100644 > > > --- a/drivers/pmdomain/core.c > > > +++ b/drivers/pmdomain/core.c > > > @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) > > > } > > > EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); > > > > > > +/** > > > + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. > > > > This isn't proper kernel-doc > > Sorry, I didn't quite get that. What is wrong? > https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation says that there should be () after the function name, and below there should be a Return: > > > > > + * > > > + * @dev: Device for which the HW-mode should be changed. > > > + * @enable: Value to set or unset the HW-mode. > > > + * > > > + * Some PM domains can rely on HW signals to control the power for a device. To > > > + * allow a consumer driver to switch the behaviour for its device in runtime, > > > + * which may be beneficial from a latency or energy point of view, this function > > > + * may be called. > > > + * > > > + * It is assumed that the users guarantee that the genpd wouldn't be detached > > > + * while this routine is getting called. > > > + * > > > + * Returns 0 on success and negative error values on failures. > > > + */ > > > +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) > > > +{ > > > + struct generic_pm_domain *genpd; > > > + int ret = 0; > > > + > > > + genpd = dev_to_genpd_safe(dev); > > > + if (!genpd) > > > + return -ENODEV; > > > + > > > + if (!genpd->set_hwmode_dev) > > > + return -EOPNOTSUPP; > > > + > > > + genpd_lock(genpd); > > > + > > > + if (dev_gpd_data(dev)->hw_mode == enable) > > > > Between this and the gdsc patch, the hw_mode state might not match the > > hardware state at boot. > > > > With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, > > false) will not bring control to SW - which might be fatal. > > Right, good point. > > I think we have two ways to deal with this: > 1) If the provider is supporting ->get_hwmode_dev(), we can let > genpd_add_device() invoke it to synchronize the state. I'd suggest that we skip the optimization for now and just let the update hit the driver on each call. > 2) If the provider doesn't support ->get_hwmode_dev() we need to call > ->set_hwmode_dev() to allow an initial state to be set. > > The question is then, if we need to allow ->get_hwmode_dev() to be > optional, if the ->set_hwmode_dev() is supported - or if we can > require it. What's your thoughts around this? > Iiuc this resource can be shared between multiple clients, and we're in either case returning the shared state. That would mean a client acting upon the returned value, is subject to races. I'm therefore inclined to say that we shouldn't have a getter, other than for debugging purposes, in which case reading the HW-state or failing would be reasonable outcomes. Regards, Bjorn
On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote: > > On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: > > On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: > > > > > > On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: > > > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > > > Some power-domains may be capable of relying on the HW to control the power > > > > for a device that's hooked up to it. Typically, for these kinds of > > > > configurations the consumer driver should be able to change the behavior of > > > > power domain at runtime, control the power domain in SW mode for certain > > > > configurations and handover the control to HW mode for other usecases. > > > > > > > > To allow a consumer driver to change the behaviour of the PM domain for its > > > > device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, > > > > let's add a corresponding optional genpd callback, ->set_hwmode_dev(), > > > > which the genpd provider should implement if it can support switching > > > > between HW controlled mode and SW controlled mode. Similarly, add the > > > > dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and > > > > its corresponding optional genpd callback, ->get_hwmode_dev(), which the > > > > genpd provider can also implement for reading back the mode from the > > > > hardware. > > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/pm_domain.h | 17 ++++++++++++ > > > > 2 files changed, 86 insertions(+) > > > > > > > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > > > > index a1f6cba3ae6c..41b6411d0ef5 100644 > > > > --- a/drivers/pmdomain/core.c > > > > +++ b/drivers/pmdomain/core.c > > > > @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) > > > > } > > > > EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); > > > > > > > > +/** > > > > + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. > > > > > > This isn't proper kernel-doc > > > > Sorry, I didn't quite get that. What is wrong? > > > > https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation > says that there should be () after the function name, and below there > should be a Return: Thanks for the pointers! > > > > > > > > + * > > > > + * @dev: Device for which the HW-mode should be changed. > > > > + * @enable: Value to set or unset the HW-mode. > > > > + * > > > > + * Some PM domains can rely on HW signals to control the power for a device. To > > > > + * allow a consumer driver to switch the behaviour for its device in runtime, > > > > + * which may be beneficial from a latency or energy point of view, this function > > > > + * may be called. > > > > + * > > > > + * It is assumed that the users guarantee that the genpd wouldn't be detached > > > > + * while this routine is getting called. > > > > + * > > > > + * Returns 0 on success and negative error values on failures. > > > > + */ > > > > +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) > > > > +{ > > > > + struct generic_pm_domain *genpd; > > > > + int ret = 0; > > > > + > > > > + genpd = dev_to_genpd_safe(dev); > > > > + if (!genpd) > > > > + return -ENODEV; > > > > + > > > > + if (!genpd->set_hwmode_dev) > > > > + return -EOPNOTSUPP; > > > > + > > > > + genpd_lock(genpd); > > > > + > > > > + if (dev_gpd_data(dev)->hw_mode == enable) > > > > > > Between this and the gdsc patch, the hw_mode state might not match the > > > hardware state at boot. > > > > > > With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, > > > false) will not bring control to SW - which might be fatal. > > > > Right, good point. > > > > I think we have two ways to deal with this: > > 1) If the provider is supporting ->get_hwmode_dev(), we can let > > genpd_add_device() invoke it to synchronize the state. > > I'd suggest that we skip the optimization for now and just let the > update hit the driver on each call. Okay. > > > 2) If the provider doesn't support ->get_hwmode_dev() we need to call > > ->set_hwmode_dev() to allow an initial state to be set. > > > > The question is then, if we need to allow ->get_hwmode_dev() to be > > optional, if the ->set_hwmode_dev() is supported - or if we can > > require it. What's your thoughts around this? > > > > Iiuc this resource can be shared between multiple clients, and we're > in either case returning the shared state. That would mean a client > acting upon the returned value, is subject to races. Not sure I understand this, but I also don't have in-depth knowledge of how the HW works. Isn't the HW mode set on a per device basis? > > I'm therefore inclined to say that we shouldn't have a getter, other > than for debugging purposes, in which case reading the HW-state or > failing would be reasonable outcomes. If you only want this for debug purposes, it seems better to keep it closer to the rpmh code, rather than adding generic callbacks to the genpd interface. So to conclude, you think having a ->set_hwmode_dev() callback should be sufficient and no caching of the current state? Abel, what's your thoughts around this? Kind regards Uffe
On 1/31/2024 4:30 AM, Bjorn Andersson wrote: > On Mon, Jan 22, 2024 at 10:47:03AM +0200, Abel Vesa wrote: >> From: Jagadeesh Kona <quic_jkona@quicinc.com> >> >> Add support for set and get hwmode callbacks to switch the GDSC between >> SW and HW modes. Currently, the GDSC is moved to HW control mode >> using HW_CTRL flag and if this flag is present, GDSC is moved to HW >> mode as part of GDSC enable itself. The intention is to keep the >> HW_CTRL flag functionality as is, since many older chipsets still use >> this flag. >> > > This provides insight into why we end up with both HW_CTRL and > HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but > rather just an implementation detail. > >> But consumer drivers also require the GDSC mode to be switched dynamically >> at runtime based on requirement for certain usecases. Some of these >> usecases are switching the GDSC to SW mode to keep it ON during the >> enablement of clocks that are dependent on GDSC and while programming >> certain configurations that require GDSC to be ON. Introduce a new >> HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev >> callbacks which allows the consumer drivers to switch the GDSC back and >> forth between HW/SW modes dynamically at runtime using new >> dev_pm_genpd_set_hwmode API. >> > > This still expresses the need for HW_CTRL_TRIGGER in terms of "some > drivers need for some use case". We don't need these many words to say: > "Introduce HW_CTRL_TRIGGER for client drivers that need it." > Thanks Bjorn for your review. Sure will update the commit text to be more precise in next series. > > I find that it would be useful to document that every time a GDSC is > turned on the mode will be switched to SW... > >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >> --- >> drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/clk/qcom/gdsc.h | 1 + >> 2 files changed, 55 insertions(+) >> >> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >> index 5358e28122ab..71626eb20101 100644 >> --- a/drivers/clk/qcom/gdsc.c >> +++ b/drivers/clk/qcom/gdsc.c >> @@ -363,6 +363,56 @@ static int gdsc_disable(struct generic_pm_domain *domain) >> return 0; >> } >> >> +static int gdsc_set_hwmode(struct generic_pm_domain *domain, struct device *dev, bool mode) >> +{ >> + struct gdsc *sc = domain_to_gdsc(domain); >> + u32 val; >> + int ret; >> + >> + if (sc->rsupply && !regulator_is_enabled(sc->rsupply)) { > > Why is this a restriction only for GDSCs supplied by regulators? I don't > find anything preventing this API from being called on GDSCs supplied by > other genpd instances. > > Also note that regulator_is_enabled() is racy, in that it tells us if > the regulator is currently turned on, not if we're the one holding that > vote. As such this might change at any moment - and hence shouldn't be > significant here. > Below is the consumer's sequence that switch the GDSC's b/w HW & SW modes:- 1) Enable the GDSC in SW mode 2) Enable required clocks 3) Switch the GDSC to HW mode using dev_pm_genpd_set_hwmode(true) 4) Usecase start 5) Usecase end 6) Switch the GDSC back to SW mode using dev_pm_genpd_set_hwmode(false) 7) Disable clocks 8) Disable GDSC Hence the new API dev_pm_genpd_set_hwmode() will always be called between gdsc_enable() and gdsc_disable(), which ensures GDSC's parent power domain/regulator is ON when this callback is being called. Also, we can remove the above regulator_is_enabled() check as well. >> + pr_err("Cannot set mode while parent is disabled\n"); >> + return -EIO; >> + } >> + >> + ret = gdsc_hwctrl(sc, mode); >> + if (ret) >> + return ret; >> + >> + /* Wait for 1usec for mode transition to properly complete */ >> + udelay(1); >> + >> + if (!mode) { >> + ret = regmap_read(sc->regmap, sc->gdscr, &val); >> + if (ret) >> + return ret; >> + >> + /* >> + * While switching from HW to SW mode, if GDSC is in enabled >> + * state, poll for GDSC to complete the power up. >> + */ > > I had to give this some thought, to conclude that this is relevant if HW > has the GDSC disabled and we're switching to SW - which would then > enable it. I think this comment can be improved slightly, to save the > reader the need for figuring out this on their own. > Sure, I will improvise the comment in next series. >> + if (!(val & SW_COLLAPSE_MASK)) > > This not being true, would imply that gdsc_disable() has been called > already, in which case there's no guarantee that the parent still > supplies power. > > In the introduced API power on and hw control are orthogonal states, but > not so in this implementation. This need to made clear, to reduce future > surprises. > Yes, above SW_COLLAPSE_MASK check is also not required and will remove it in next series. >> + return gdsc_poll_status(sc, GDSC_ON); >> + } >> + >> + return 0; >> +} >> + >> +static bool gdsc_get_hwmode(struct generic_pm_domain *domain, struct device *dev) >> +{ >> + struct gdsc *sc = domain_to_gdsc(domain); >> + u32 val; >> + int ret; >> + >> + ret = regmap_read(sc->regmap, sc->gdscr, &val); >> + if (ret) >> + return ret; >> + >> + if (val & HW_CONTROL_MASK) >> + return true; >> + >> + return false; > > return !!(val & HW_CONTROL_MASK); > Sure, will update this in the next series. > Regards, > Bjorn > >> +} >> + >> static int gdsc_init(struct gdsc *sc) >> { >> u32 mask, val; >> @@ -451,6 +501,10 @@ static int gdsc_init(struct gdsc *sc) >> sc->pd.power_off = gdsc_disable; >> if (!sc->pd.power_on) >> sc->pd.power_on = gdsc_enable; >> + if (sc->flags & HW_CTRL_TRIGGER) { >> + sc->pd.set_hwmode_dev = gdsc_set_hwmode; >> + sc->pd.get_hwmode_dev = gdsc_get_hwmode; >> + } >> >> ret = pm_genpd_init(&sc->pd, NULL, !on); >> if (ret) >> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h >> index 803512688336..1e2779b823d1 100644 >> --- a/drivers/clk/qcom/gdsc.h >> +++ b/drivers/clk/qcom/gdsc.h >> @@ -67,6 +67,7 @@ struct gdsc { >> #define ALWAYS_ON BIT(6) >> #define RETAIN_FF_ENABLE BIT(7) >> #define NO_RET_PERIPH BIT(8) >> +#define HW_CTRL_TRIGGER BIT(9) >> struct reset_controller_dev *rcdev; >> unsigned int *resets; >> unsigned int reset_count; >> >> -- >> 2.34.1 >> >
On 1/31/2024 6:35 AM, Bjorn Andersson wrote: > On Mon, Jan 22, 2024 at 10:47:05AM +0200, Abel Vesa wrote: >> From: Jagadeesh Kona <quic_jkona@quicinc.com> >> >> Use dev_pm_genpd_set_hwmode API to switch the vcodec gdsc to SW/HW >> modes at runtime based on requirement for venus V6 variants. >> >> Before the GDSC HWCTL was available to the consumer, the venus driver >> needed to somehow keep the power from collapsing while under the driver >> control. The only way to do that was to clear the CORE_PWR_DISABLE bit >> (in wrapper POWER_CONTROL register) and, respectively, set it back after >> the driver control was completed. Now, that there is a way to switch the >> GDSC HW/SW control back and forth, the CORE_PWR_DISABLE toggling in >> vcodec_control_v4() can be dropped for V6 variants. >> > > The purpose of this commit is to warrant the need of this new mechanism, > but I don't find that it actually describes a problem to be solved. > >> With newer implementation, the mode of vcodec gdsc gets switched only in > > Does "With newer implementation" mean "after these patches are applied"? > Thanks Bjorn for your review! Yes, after all these patches are applied, will update the commit text to be bit more precise. >> set_hwmode API and the GDSC should not be switched to HW control mode >> before turning off the GDSC, else subsequent GDSC enable may fail, hence >> add check to avoid switching the GDSC to HW mode before powering off the >> GDSC on V6 variants. >> > > Is this saying that "if we return the GDSC to HW control after turning > off the clocks, it might not be possible to turn it on again"? > Yes, if the GDSC is left in HW control mode before GDSC disable, the subsequent GDSC enable callback may fail while polling for GDSC status, since HW can keep the GDSC in disabled state. > How come? Today this GDSC is operating in HW control mode, before, > during and after the clock operation. > Currently once GDSC is moved to HW control mode, Venus driver is using it's POWER_CONTROL register to keep the GDSC ON before the clock operations and reset it back after clock operations to handover control back to HW. And these venus POWER_CONTROL register addresses are not constant and vary from one venus variant to other. With this new API Venus driver can avoid these register writes and use this standard API everywhere to switch the GDSC mode as required. >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >> --- >> drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++++---------- >> 1 file changed, 13 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c >> index a1b127caa90a..55e8ec3f4ee9 100644 >> --- a/drivers/media/platform/qcom/venus/pm_helpers.c >> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c >> @@ -412,10 +412,9 @@ static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable) >> u32 val; >> int ret; >> >> - if (IS_V6(core)) { >> - ctrl = core->wrapper_base + WRAPPER_CORE_POWER_CONTROL_V6; >> - stat = core->wrapper_base + WRAPPER_CORE_POWER_STATUS_V6; >> - } else if (coreid == VIDC_CORE_ID_1) { >> + if (IS_V6(core)) >> + return dev_pm_genpd_set_hwmode(core->pmdomains[coreid], !enable); >> + else if (coreid == VIDC_CORE_ID_1) { >> ctrl = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_CONTROL; >> stat = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_STATUS; >> } else { >> @@ -451,9 +450,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask) >> >> vcodec_clks_disable(core, core->vcodec0_clks); >> >> - ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false); >> - if (ret) >> - return ret; >> + if (!IS_V6(core)) { >> + ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false); > > First I had this expectation that the GDSC will always be in SW control > when the GDSC turns on - like the downstream implementation. > > In this case I felt we should have a similar condition in > poweron_coreid() - as there's no point in switching to SW mode when we > know we're in SW mode already. > > > But as I finally realized that this is not the case, I now see that by > skipping the transition to HW mode here, dev_pm_genpd_set_hwmode() will > find the domain in SW mode, and through > > if (dev_gpd_data(dev)->hw_mode == enable) > > Will turn the vcodec_control_v4(, true) into a nop. > > So, my first first instinct of feeling that this should be symmetric > between poweron/poweroff was reasonable...I think... > Yes, we can add similar check in poweron_coreid() also to be symmetric but since it will be nop haven't added it. Shall I add similar check in poweron_coreid() as well? > > I find that this interface does not match the expectations that people > will bring from downstream and this example isn't helpful in explaining > how to use the new interface. > There are 3 consumers that currently use this HW control mode for GDSC's:- display, camera and display. Display driver is able to operate with GDSC always in HW mode. Camera drivers don't have power saving features enabled on upstream yet and hence not using the HW control mode of GDSC's currently, but will need this API support to enable camera power saving features on upstream. Currently on upstream, only venus driver requires GDSC HW and SW modes switching, and hence added support in this driver to use the new interface. > PS. I trust there's no case whre legacy_binding = true, or that that > code path does not need similar workaround? > This change is applicable only to sc7280 and sm8250 targets for which legacy_binding will be false. Thanks, Jagadeesh > Regards, > Bjorn > >> + if (ret) >> + return ret; >> + } >> >> ret = pm_runtime_put_sync(core->pmdomains[1]); >> if (ret < 0) >> @@ -467,9 +468,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask) >> >> vcodec_clks_disable(core, core->vcodec1_clks); >> >> - ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false); >> - if (ret) >> - return ret; >> + if (!IS_V6(core)) { >> + ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false); >> + if (ret) >> + return ret; >> + } >> >> ret = pm_runtime_put_sync(core->pmdomains[2]); >> if (ret < 0) >> >> -- >> 2.34.1 >>
On 1/31/2024 5:49 AM, Bjorn Andersson wrote: > On Tue, Jan 30, 2024 at 05:00:28PM -0600, Bjorn Andersson wrote: >> On Mon, Jan 22, 2024 at 10:47:03AM +0200, Abel Vesa wrote: >>> From: Jagadeesh Kona <quic_jkona@quicinc.com> >>> >>> Add support for set and get hwmode callbacks to switch the GDSC between >>> SW and HW modes. Currently, the GDSC is moved to HW control mode >>> using HW_CTRL flag and if this flag is present, GDSC is moved to HW >>> mode as part of GDSC enable itself. The intention is to keep the >>> HW_CTRL flag functionality as is, since many older chipsets still use >>> this flag. >>> >> >> This provides insight into why we end up with both HW_CTRL and >> HW_CTRL_TRIGGER. This doesn't describe why this change is needed, but >> rather just an implementation detail. >> >>> But consumer drivers also require the GDSC mode to be switched dynamically >>> at runtime based on requirement for certain usecases. Some of these >>> usecases are switching the GDSC to SW mode to keep it ON during the >>> enablement of clocks that are dependent on GDSC and while programming >>> certain configurations that require GDSC to be ON. Introduce a new >>> HW_CTRL_TRIGGER flag to register the set_hwmode_dev and get_hwmode_dev >>> callbacks which allows the consumer drivers to switch the GDSC back and >>> forth between HW/SW modes dynamically at runtime using new >>> dev_pm_genpd_set_hwmode API. >>> >> >> This still expresses the need for HW_CTRL_TRIGGER in terms of "some >> drivers need for some use case". We don't need these many words to say: >> "Introduce HW_CTRL_TRIGGER for client drivers that need it." >> >> >> I find that it would be useful to document that every time a GDSC is >> turned on the mode will be switched to SW... >> > > I believe I'm wrong here. Reading the patch again, I think we might > retain the mode across a disable/enable cycle. I at least don't see > anything explicit returning us to SW mode. > > According to Linux though, the GDSC is off, so as described below, there > will be no votes for supplying resources. > Yes, With HW_CTRL_TRIGGER flag, the GDSC mode gets switched only in new set_hwmode_dev() callback when consumers explicitly call for it. This is to ensure GenPD f/w's hwmode aligns with the actual GDSC mode in HW. With the new API, the expectation is consumer drivers should move the GDSC to SW control mode before disabling the GDSC, which ensures that, GDSC is turned ON in SW mode every time. Thanks, Jagadeesh > Regards, > Bjorn
On 2/2/2024 5:59 PM, Ulf Hansson wrote: > On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote: >> >> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: >>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: >>>> >>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: >>>>> From: Ulf Hansson <ulf.hansson@linaro.org> >>>>> >>>>> Some power-domains may be capable of relying on the HW to control the power >>>>> for a device that's hooked up to it. Typically, for these kinds of >>>>> configurations the consumer driver should be able to change the behavior of >>>>> power domain at runtime, control the power domain in SW mode for certain >>>>> configurations and handover the control to HW mode for other usecases. >>>>> >>>>> To allow a consumer driver to change the behaviour of the PM domain for its >>>>> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, >>>>> let's add a corresponding optional genpd callback, ->set_hwmode_dev(), >>>>> which the genpd provider should implement if it can support switching >>>>> between HW controlled mode and SW controlled mode. Similarly, add the >>>>> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and >>>>> its corresponding optional genpd callback, ->get_hwmode_dev(), which the >>>>> genpd provider can also implement for reading back the mode from the >>>>> hardware. >>>>> >>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/pm_domain.h | 17 ++++++++++++ >>>>> 2 files changed, 86 insertions(+) >>>>> >>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c >>>>> index a1f6cba3ae6c..41b6411d0ef5 100644 >>>>> --- a/drivers/pmdomain/core.c >>>>> +++ b/drivers/pmdomain/core.c >>>>> @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) >>>>> } >>>>> EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); >>>>> >>>>> +/** >>>>> + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. >>>> >>>> This isn't proper kernel-doc >>> >>> Sorry, I didn't quite get that. What is wrong? >>> >> >> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation >> says that there should be () after the function name, and below there >> should be a Return: > > Thanks for the pointers! > >> >>>> >>>>> + * >>>>> + * @dev: Device for which the HW-mode should be changed. >>>>> + * @enable: Value to set or unset the HW-mode. >>>>> + * >>>>> + * Some PM domains can rely on HW signals to control the power for a device. To >>>>> + * allow a consumer driver to switch the behaviour for its device in runtime, >>>>> + * which may be beneficial from a latency or energy point of view, this function >>>>> + * may be called. >>>>> + * >>>>> + * It is assumed that the users guarantee that the genpd wouldn't be detached >>>>> + * while this routine is getting called. >>>>> + * >>>>> + * Returns 0 on success and negative error values on failures. >>>>> + */ >>>>> +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) >>>>> +{ >>>>> + struct generic_pm_domain *genpd; >>>>> + int ret = 0; >>>>> + >>>>> + genpd = dev_to_genpd_safe(dev); >>>>> + if (!genpd) >>>>> + return -ENODEV; >>>>> + >>>>> + if (!genpd->set_hwmode_dev) >>>>> + return -EOPNOTSUPP; >>>>> + >>>>> + genpd_lock(genpd); >>>>> + >>>>> + if (dev_gpd_data(dev)->hw_mode == enable) >>>> >>>> Between this and the gdsc patch, the hw_mode state might not match the >>>> hardware state at boot. >>>> >>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, >>>> false) will not bring control to SW - which might be fatal. >>> >>> Right, good point. >>> >>> I think we have two ways to deal with this: >>> 1) If the provider is supporting ->get_hwmode_dev(), we can let >>> genpd_add_device() invoke it to synchronize the state. >> >> I'd suggest that we skip the optimization for now and just let the >> update hit the driver on each call. > > Okay. > >> >>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call >>> ->set_hwmode_dev() to allow an initial state to be set. >>> >>> The question is then, if we need to allow ->get_hwmode_dev() to be >>> optional, if the ->set_hwmode_dev() is supported - or if we can >>> require it. What's your thoughts around this? >>> >> >> Iiuc this resource can be shared between multiple clients, and we're >> in either case returning the shared state. That would mean a client >> acting upon the returned value, is subject to races. > > Not sure I understand this, but I also don't have in-depth knowledge > of how the HW works. > > Isn't the HW mode set on a per device basis? > >> >> I'm therefore inclined to say that we shouldn't have a getter, other >> than for debugging purposes, in which case reading the HW-state or >> failing would be reasonable outcomes. > > If you only want this for debug purposes, it seems better to keep it > closer to the rpmh code, rather than adding generic callbacks to the > genpd interface. > > So to conclude, you think having a ->set_hwmode_dev() callback should > be sufficient and no caching of the current state? > > Abel, what's your thoughts around this? > We believe it is good to have get_hwmode_dev() callback supported from GenPD, since if multiple devices share a GenPD, and if one device moves the GenPD to HW mode, the other device won't be aware of it and second device's dev_gpd_data(dev)->hw_mode will still be false. If we have this dev_pm_genpd_get_hwmode() API supported and if we assign dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev() callback, consumer drivers can use this API to sync the actual HW mode of the GenPD. Thanks, Jagadeesh > Kind regards > Uffe
On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 2/2/2024 5:59 PM, Ulf Hansson wrote: > > On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote: > >> > >> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: > >>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: > >>>> > >>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: > >>>>> From: Ulf Hansson <ulf.hansson@linaro.org> > >>>>> > >>>>> Some power-domains may be capable of relying on the HW to control the power > >>>>> for a device that's hooked up to it. Typically, for these kinds of > >>>>> configurations the consumer driver should be able to change the behavior of > >>>>> power domain at runtime, control the power domain in SW mode for certain > >>>>> configurations and handover the control to HW mode for other usecases. > >>>>> > >>>>> To allow a consumer driver to change the behaviour of the PM domain for its > >>>>> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, > >>>>> let's add a corresponding optional genpd callback, ->set_hwmode_dev(), > >>>>> which the genpd provider should implement if it can support switching > >>>>> between HW controlled mode and SW controlled mode. Similarly, add the > >>>>> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and > >>>>> its corresponding optional genpd callback, ->get_hwmode_dev(), which the > >>>>> genpd provider can also implement for reading back the mode from the > >>>>> hardware. > >>>>> > >>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > >>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>>> --- > >>>>> drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ > >>>>> include/linux/pm_domain.h | 17 ++++++++++++ > >>>>> 2 files changed, 86 insertions(+) > >>>>> > >>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > >>>>> index a1f6cba3ae6c..41b6411d0ef5 100644 > >>>>> --- a/drivers/pmdomain/core.c > >>>>> +++ b/drivers/pmdomain/core.c > >>>>> @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) > >>>>> } > >>>>> EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); > >>>>> > >>>>> +/** > >>>>> + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. > >>>> > >>>> This isn't proper kernel-doc > >>> > >>> Sorry, I didn't quite get that. What is wrong? > >>> > >> > >> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation > >> says that there should be () after the function name, and below there > >> should be a Return: > > > > Thanks for the pointers! > > > >> > >>>> > >>>>> + * > >>>>> + * @dev: Device for which the HW-mode should be changed. > >>>>> + * @enable: Value to set or unset the HW-mode. > >>>>> + * > >>>>> + * Some PM domains can rely on HW signals to control the power for a device. To > >>>>> + * allow a consumer driver to switch the behaviour for its device in runtime, > >>>>> + * which may be beneficial from a latency or energy point of view, this function > >>>>> + * may be called. > >>>>> + * > >>>>> + * It is assumed that the users guarantee that the genpd wouldn't be detached > >>>>> + * while this routine is getting called. > >>>>> + * > >>>>> + * Returns 0 on success and negative error values on failures. > >>>>> + */ > >>>>> +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) > >>>>> +{ > >>>>> + struct generic_pm_domain *genpd; > >>>>> + int ret = 0; > >>>>> + > >>>>> + genpd = dev_to_genpd_safe(dev); > >>>>> + if (!genpd) > >>>>> + return -ENODEV; > >>>>> + > >>>>> + if (!genpd->set_hwmode_dev) > >>>>> + return -EOPNOTSUPP; > >>>>> + > >>>>> + genpd_lock(genpd); > >>>>> + > >>>>> + if (dev_gpd_data(dev)->hw_mode == enable) > >>>> > >>>> Between this and the gdsc patch, the hw_mode state might not match the > >>>> hardware state at boot. > >>>> > >>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, > >>>> false) will not bring control to SW - which might be fatal. > >>> > >>> Right, good point. > >>> > >>> I think we have two ways to deal with this: > >>> 1) If the provider is supporting ->get_hwmode_dev(), we can let > >>> genpd_add_device() invoke it to synchronize the state. > >> > >> I'd suggest that we skip the optimization for now and just let the > >> update hit the driver on each call. > > > > Okay. > > > >> > >>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call > >>> ->set_hwmode_dev() to allow an initial state to be set. > >>> > >>> The question is then, if we need to allow ->get_hwmode_dev() to be > >>> optional, if the ->set_hwmode_dev() is supported - or if we can > >>> require it. What's your thoughts around this? > >>> > >> > >> Iiuc this resource can be shared between multiple clients, and we're > >> in either case returning the shared state. That would mean a client > >> acting upon the returned value, is subject to races. > > > > Not sure I understand this, but I also don't have in-depth knowledge > > of how the HW works. > > > > Isn't the HW mode set on a per device basis? > > > >> > >> I'm therefore inclined to say that we shouldn't have a getter, other > >> than for debugging purposes, in which case reading the HW-state or > >> failing would be reasonable outcomes. > > > > If you only want this for debug purposes, it seems better to keep it > > closer to the rpmh code, rather than adding generic callbacks to the > > genpd interface. > > > > So to conclude, you think having a ->set_hwmode_dev() callback should > > be sufficient and no caching of the current state? > > > > Abel, what's your thoughts around this? > > > > We believe it is good to have get_hwmode_dev() callback supported from > GenPD, since if multiple devices share a GenPD, and if one device moves > the GenPD to HW mode, the other device won't be aware of it and second > device's dev_gpd_data(dev)->hw_mode will still be false. > > If we have this dev_pm_genpd_get_hwmode() API supported and if we assign > dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev() > callback, consumer drivers can use this API to sync the actual HW mode > of the GenPD. Hmm, I thought the HW mode was being set on a per device basis, via its PM domain. Did I get that wrong? Are you saying there could be multiple devices sharing the same PM domain and thus also sharing the same HW mode? In that case, it sure sounds like we have synchronization issues to deal with too. Kind regards Uffe
On 2/13/2024 7:21 PM, Ulf Hansson wrote: > On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 2/2/2024 5:59 PM, Ulf Hansson wrote: >>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote: >>>> >>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: >>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: >>>>>> >>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: >>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org> >>>>>>> >>>>>>> Some power-domains may be capable of relying on the HW to control the power >>>>>>> for a device that's hooked up to it. Typically, for these kinds of >>>>>>> configurations the consumer driver should be able to change the behavior of >>>>>>> power domain at runtime, control the power domain in SW mode for certain >>>>>>> configurations and handover the control to HW mode for other usecases. >>>>>>> >>>>>>> To allow a consumer driver to change the behaviour of the PM domain for its >>>>>>> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, >>>>>>> let's add a corresponding optional genpd callback, ->set_hwmode_dev(), >>>>>>> which the genpd provider should implement if it can support switching >>>>>>> between HW controlled mode and SW controlled mode. Similarly, add the >>>>>>> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and >>>>>>> its corresponding optional genpd callback, ->get_hwmode_dev(), which the >>>>>>> genpd provider can also implement for reading back the mode from the >>>>>>> hardware. >>>>>>> >>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>>> --- >>>>>>> drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>> include/linux/pm_domain.h | 17 ++++++++++++ >>>>>>> 2 files changed, 86 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c >>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644 >>>>>>> --- a/drivers/pmdomain/core.c >>>>>>> +++ b/drivers/pmdomain/core.c >>>>>>> @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) >>>>>>> } >>>>>>> EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); >>>>>>> >>>>>>> +/** >>>>>>> + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. >>>>>> >>>>>> This isn't proper kernel-doc >>>>> >>>>> Sorry, I didn't quite get that. What is wrong? >>>>> >>>> >>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation >>>> says that there should be () after the function name, and below there >>>> should be a Return: >>> >>> Thanks for the pointers! >>> >>>> >>>>>> >>>>>>> + * >>>>>>> + * @dev: Device for which the HW-mode should be changed. >>>>>>> + * @enable: Value to set or unset the HW-mode. >>>>>>> + * >>>>>>> + * Some PM domains can rely on HW signals to control the power for a device. To >>>>>>> + * allow a consumer driver to switch the behaviour for its device in runtime, >>>>>>> + * which may be beneficial from a latency or energy point of view, this function >>>>>>> + * may be called. >>>>>>> + * >>>>>>> + * It is assumed that the users guarantee that the genpd wouldn't be detached >>>>>>> + * while this routine is getting called. >>>>>>> + * >>>>>>> + * Returns 0 on success and negative error values on failures. >>>>>>> + */ >>>>>>> +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) >>>>>>> +{ >>>>>>> + struct generic_pm_domain *genpd; >>>>>>> + int ret = 0; >>>>>>> + >>>>>>> + genpd = dev_to_genpd_safe(dev); >>>>>>> + if (!genpd) >>>>>>> + return -ENODEV; >>>>>>> + >>>>>>> + if (!genpd->set_hwmode_dev) >>>>>>> + return -EOPNOTSUPP; >>>>>>> + >>>>>>> + genpd_lock(genpd); >>>>>>> + >>>>>>> + if (dev_gpd_data(dev)->hw_mode == enable) >>>>>> >>>>>> Between this and the gdsc patch, the hw_mode state might not match the >>>>>> hardware state at boot. >>>>>> >>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, >>>>>> false) will not bring control to SW - which might be fatal. >>>>> >>>>> Right, good point. >>>>> >>>>> I think we have two ways to deal with this: >>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let >>>>> genpd_add_device() invoke it to synchronize the state. >>>> >>>> I'd suggest that we skip the optimization for now and just let the >>>> update hit the driver on each call. >>> >>> Okay. >>> >>>> >>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call >>>>> ->set_hwmode_dev() to allow an initial state to be set. >>>>> >>>>> The question is then, if we need to allow ->get_hwmode_dev() to be >>>>> optional, if the ->set_hwmode_dev() is supported - or if we can >>>>> require it. What's your thoughts around this? >>>>> >>>> >>>> Iiuc this resource can be shared between multiple clients, and we're >>>> in either case returning the shared state. That would mean a client >>>> acting upon the returned value, is subject to races. >>> >>> Not sure I understand this, but I also don't have in-depth knowledge >>> of how the HW works. >>> >>> Isn't the HW mode set on a per device basis? >>> >>>> >>>> I'm therefore inclined to say that we shouldn't have a getter, other >>>> than for debugging purposes, in which case reading the HW-state or >>>> failing would be reasonable outcomes. >>> >>> If you only want this for debug purposes, it seems better to keep it >>> closer to the rpmh code, rather than adding generic callbacks to the >>> genpd interface. >>> >>> So to conclude, you think having a ->set_hwmode_dev() callback should >>> be sufficient and no caching of the current state? >>> >>> Abel, what's your thoughts around this? >>> >> >> We believe it is good to have get_hwmode_dev() callback supported from >> GenPD, since if multiple devices share a GenPD, and if one device moves >> the GenPD to HW mode, the other device won't be aware of it and second >> device's dev_gpd_data(dev)->hw_mode will still be false. >> >> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign >> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev() >> callback, consumer drivers can use this API to sync the actual HW mode >> of the GenPD. > > Hmm, I thought the HW mode was being set on a per device basis, via > its PM domain. Did I get that wrong? > > Are you saying there could be multiple devices sharing the same PM > domain and thus also sharing the same HW mode? In that case, it sure > sounds like we have synchronization issues to deal with too. > Sorry my bad, currently we don't have usecase where multiple devices sharing the same PM domain that have HW control support, so there is no synchronization issue. But it would be good to have .get_hwmode_dev() callback for consumer drivers to query the actual GenPD mode from HW, whenever they require it. Thanks, Jagadeesh
On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 2/13/2024 7:21 PM, Ulf Hansson wrote: > > On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >> > >> > >> > >> On 2/2/2024 5:59 PM, Ulf Hansson wrote: > >>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote: > >>>> > >>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: > >>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: > >>>>>> > >>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: > >>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org> > >>>>>>> > >>>>>>> Some power-domains may be capable of relying on the HW to control the power > >>>>>>> for a device that's hooked up to it. Typically, for these kinds of > >>>>>>> configurations the consumer driver should be able to change the behavior of > >>>>>>> power domain at runtime, control the power domain in SW mode for certain > >>>>>>> configurations and handover the control to HW mode for other usecases. > >>>>>>> > >>>>>>> To allow a consumer driver to change the behaviour of the PM domain for its > >>>>>>> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, > >>>>>>> let's add a corresponding optional genpd callback, ->set_hwmode_dev(), > >>>>>>> which the genpd provider should implement if it can support switching > >>>>>>> between HW controlled mode and SW controlled mode. Similarly, add the > >>>>>>> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and > >>>>>>> its corresponding optional genpd callback, ->get_hwmode_dev(), which the > >>>>>>> genpd provider can also implement for reading back the mode from the > >>>>>>> hardware. > >>>>>>> > >>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >>>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > >>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>>>>> --- > >>>>>>> drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ > >>>>>>> include/linux/pm_domain.h | 17 ++++++++++++ > >>>>>>> 2 files changed, 86 insertions(+) > >>>>>>> > >>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > >>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644 > >>>>>>> --- a/drivers/pmdomain/core.c > >>>>>>> +++ b/drivers/pmdomain/core.c > >>>>>>> @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) > >>>>>>> } > >>>>>>> EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); > >>>>>>> > >>>>>>> +/** > >>>>>>> + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. > >>>>>> > >>>>>> This isn't proper kernel-doc > >>>>> > >>>>> Sorry, I didn't quite get that. What is wrong? > >>>>> > >>>> > >>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation > >>>> says that there should be () after the function name, and below there > >>>> should be a Return: > >>> > >>> Thanks for the pointers! > >>> > >>>> > >>>>>> > >>>>>>> + * > >>>>>>> + * @dev: Device for which the HW-mode should be changed. > >>>>>>> + * @enable: Value to set or unset the HW-mode. > >>>>>>> + * > >>>>>>> + * Some PM domains can rely on HW signals to control the power for a device. To > >>>>>>> + * allow a consumer driver to switch the behaviour for its device in runtime, > >>>>>>> + * which may be beneficial from a latency or energy point of view, this function > >>>>>>> + * may be called. > >>>>>>> + * > >>>>>>> + * It is assumed that the users guarantee that the genpd wouldn't be detached > >>>>>>> + * while this routine is getting called. > >>>>>>> + * > >>>>>>> + * Returns 0 on success and negative error values on failures. > >>>>>>> + */ > >>>>>>> +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) > >>>>>>> +{ > >>>>>>> + struct generic_pm_domain *genpd; > >>>>>>> + int ret = 0; > >>>>>>> + > >>>>>>> + genpd = dev_to_genpd_safe(dev); > >>>>>>> + if (!genpd) > >>>>>>> + return -ENODEV; > >>>>>>> + > >>>>>>> + if (!genpd->set_hwmode_dev) > >>>>>>> + return -EOPNOTSUPP; > >>>>>>> + > >>>>>>> + genpd_lock(genpd); > >>>>>>> + > >>>>>>> + if (dev_gpd_data(dev)->hw_mode == enable) > >>>>>> > >>>>>> Between this and the gdsc patch, the hw_mode state might not match the > >>>>>> hardware state at boot. > >>>>>> > >>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, > >>>>>> false) will not bring control to SW - which might be fatal. > >>>>> > >>>>> Right, good point. > >>>>> > >>>>> I think we have two ways to deal with this: > >>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let > >>>>> genpd_add_device() invoke it to synchronize the state. > >>>> > >>>> I'd suggest that we skip the optimization for now and just let the > >>>> update hit the driver on each call. > >>> > >>> Okay. > >>> > >>>> > >>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call > >>>>> ->set_hwmode_dev() to allow an initial state to be set. > >>>>> > >>>>> The question is then, if we need to allow ->get_hwmode_dev() to be > >>>>> optional, if the ->set_hwmode_dev() is supported - or if we can > >>>>> require it. What's your thoughts around this? > >>>>> > >>>> > >>>> Iiuc this resource can be shared between multiple clients, and we're > >>>> in either case returning the shared state. That would mean a client > >>>> acting upon the returned value, is subject to races. > >>> > >>> Not sure I understand this, but I also don't have in-depth knowledge > >>> of how the HW works. > >>> > >>> Isn't the HW mode set on a per device basis? > >>> > >>>> > >>>> I'm therefore inclined to say that we shouldn't have a getter, other > >>>> than for debugging purposes, in which case reading the HW-state or > >>>> failing would be reasonable outcomes. > >>> > >>> If you only want this for debug purposes, it seems better to keep it > >>> closer to the rpmh code, rather than adding generic callbacks to the > >>> genpd interface. > >>> > >>> So to conclude, you think having a ->set_hwmode_dev() callback should > >>> be sufficient and no caching of the current state? > >>> > >>> Abel, what's your thoughts around this? > >>> > >> > >> We believe it is good to have get_hwmode_dev() callback supported from > >> GenPD, since if multiple devices share a GenPD, and if one device moves > >> the GenPD to HW mode, the other device won't be aware of it and second > >> device's dev_gpd_data(dev)->hw_mode will still be false. > >> > >> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign > >> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev() > >> callback, consumer drivers can use this API to sync the actual HW mode > >> of the GenPD. > > > > Hmm, I thought the HW mode was being set on a per device basis, via > > its PM domain. Did I get that wrong? > > > > Are you saying there could be multiple devices sharing the same PM > > domain and thus also sharing the same HW mode? In that case, it sure > > sounds like we have synchronization issues to deal with too. > > > > Sorry my bad, currently we don't have usecase where multiple devices > sharing the same PM domain that have HW control support, so there is no > synchronization issue. Okay, good! > > But it would be good to have .get_hwmode_dev() callback for consumer > drivers to query the actual GenPD mode from HW, whenever they require it. Okay, no objection from my side. Then the final question is if we need a variable to keep a cache of the current HW mode for each device. Perhaps we should start simple and just always invoke the callbacks from genpd, what do you think? Kind regards Uffe
On 2/15/2024 9:57 PM, Ulf Hansson wrote: > On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 2/13/2024 7:21 PM, Ulf Hansson wrote: >>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote: >>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote: >>>>>> >>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: >>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: >>>>>>>> >>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: >>>>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org> >>>>>>>>> >>>>>>>>> Some power-domains may be capable of relying on the HW to control the power >>>>>>>>> for a device that's hooked up to it. Typically, for these kinds of >>>>>>>>> configurations the consumer driver should be able to change the behavior of >>>>>>>>> power domain at runtime, control the power domain in SW mode for certain >>>>>>>>> configurations and handover the control to HW mode for other usecases. >>>>>>>>> >>>>>>>>> To allow a consumer driver to change the behaviour of the PM domain for its >>>>>>>>> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, >>>>>>>>> let's add a corresponding optional genpd callback, ->set_hwmode_dev(), >>>>>>>>> which the genpd provider should implement if it can support switching >>>>>>>>> between HW controlled mode and SW controlled mode. Similarly, add the >>>>>>>>> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and >>>>>>>>> its corresponding optional genpd callback, ->get_hwmode_dev(), which the >>>>>>>>> genpd provider can also implement for reading back the mode from the >>>>>>>>> hardware. >>>>>>>>> >>>>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>>>>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>>>>> --- >>>>>>>>> drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>>> include/linux/pm_domain.h | 17 ++++++++++++ >>>>>>>>> 2 files changed, 86 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c >>>>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644 >>>>>>>>> --- a/drivers/pmdomain/core.c >>>>>>>>> +++ b/drivers/pmdomain/core.c >>>>>>>>> @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) >>>>>>>>> } >>>>>>>>> EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); >>>>>>>>> >>>>>>>>> +/** >>>>>>>>> + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. >>>>>>>> >>>>>>>> This isn't proper kernel-doc >>>>>>> >>>>>>> Sorry, I didn't quite get that. What is wrong? >>>>>>> >>>>>> >>>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation >>>>>> says that there should be () after the function name, and below there >>>>>> should be a Return: >>>>> >>>>> Thanks for the pointers! >>>>> >>>>>> >>>>>>>> >>>>>>>>> + * >>>>>>>>> + * @dev: Device for which the HW-mode should be changed. >>>>>>>>> + * @enable: Value to set or unset the HW-mode. >>>>>>>>> + * >>>>>>>>> + * Some PM domains can rely on HW signals to control the power for a device. To >>>>>>>>> + * allow a consumer driver to switch the behaviour for its device in runtime, >>>>>>>>> + * which may be beneficial from a latency or energy point of view, this function >>>>>>>>> + * may be called. >>>>>>>>> + * >>>>>>>>> + * It is assumed that the users guarantee that the genpd wouldn't be detached >>>>>>>>> + * while this routine is getting called. >>>>>>>>> + * >>>>>>>>> + * Returns 0 on success and negative error values on failures. >>>>>>>>> + */ >>>>>>>>> +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) >>>>>>>>> +{ >>>>>>>>> + struct generic_pm_domain *genpd; >>>>>>>>> + int ret = 0; >>>>>>>>> + >>>>>>>>> + genpd = dev_to_genpd_safe(dev); >>>>>>>>> + if (!genpd) >>>>>>>>> + return -ENODEV; >>>>>>>>> + >>>>>>>>> + if (!genpd->set_hwmode_dev) >>>>>>>>> + return -EOPNOTSUPP; >>>>>>>>> + >>>>>>>>> + genpd_lock(genpd); >>>>>>>>> + >>>>>>>>> + if (dev_gpd_data(dev)->hw_mode == enable) >>>>>>>> >>>>>>>> Between this and the gdsc patch, the hw_mode state might not match the >>>>>>>> hardware state at boot. >>>>>>>> >>>>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, >>>>>>>> false) will not bring control to SW - which might be fatal. >>>>>>> >>>>>>> Right, good point. >>>>>>> >>>>>>> I think we have two ways to deal with this: >>>>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let >>>>>>> genpd_add_device() invoke it to synchronize the state. >>>>>> >>>>>> I'd suggest that we skip the optimization for now and just let the >>>>>> update hit the driver on each call. >>>>> >>>>> Okay. >>>>> >>>>>> >>>>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call >>>>>>> ->set_hwmode_dev() to allow an initial state to be set. >>>>>>> >>>>>>> The question is then, if we need to allow ->get_hwmode_dev() to be >>>>>>> optional, if the ->set_hwmode_dev() is supported - or if we can >>>>>>> require it. What's your thoughts around this? >>>>>>> >>>>>> >>>>>> Iiuc this resource can be shared between multiple clients, and we're >>>>>> in either case returning the shared state. That would mean a client >>>>>> acting upon the returned value, is subject to races. >>>>> >>>>> Not sure I understand this, but I also don't have in-depth knowledge >>>>> of how the HW works. >>>>> >>>>> Isn't the HW mode set on a per device basis? >>>>> >>>>>> >>>>>> I'm therefore inclined to say that we shouldn't have a getter, other >>>>>> than for debugging purposes, in which case reading the HW-state or >>>>>> failing would be reasonable outcomes. >>>>> >>>>> If you only want this for debug purposes, it seems better to keep it >>>>> closer to the rpmh code, rather than adding generic callbacks to the >>>>> genpd interface. >>>>> >>>>> So to conclude, you think having a ->set_hwmode_dev() callback should >>>>> be sufficient and no caching of the current state? >>>>> >>>>> Abel, what's your thoughts around this? >>>>> >>>> >>>> We believe it is good to have get_hwmode_dev() callback supported from >>>> GenPD, since if multiple devices share a GenPD, and if one device moves >>>> the GenPD to HW mode, the other device won't be aware of it and second >>>> device's dev_gpd_data(dev)->hw_mode will still be false. >>>> >>>> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign >>>> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev() >>>> callback, consumer drivers can use this API to sync the actual HW mode >>>> of the GenPD. >>> >>> Hmm, I thought the HW mode was being set on a per device basis, via >>> its PM domain. Did I get that wrong? >>> >>> Are you saying there could be multiple devices sharing the same PM >>> domain and thus also sharing the same HW mode? In that case, it sure >>> sounds like we have synchronization issues to deal with too. >>> >> >> Sorry my bad, currently we don't have usecase where multiple devices >> sharing the same PM domain that have HW control support, so there is no >> synchronization issue. > > Okay, good! > >> >> But it would be good to have .get_hwmode_dev() callback for consumer >> drivers to query the actual GenPD mode from HW, whenever they require it. > > Okay, no objection from my side. > > Then the final question is if we need a variable to keep a cache of > the current HW mode for each device. Perhaps we should start simple > and just always invoke the callbacks from genpd, what do you think? > Yes, agree, we can remove the variable and just always invoke the callbacks from genpd. But we may need the variable to reflect GenPD mode in debugfs genpd_summary, or need to invoke get callback there as well to get the current mode. Thanks, Jagadeesh > Kind regards > Uffe
On Fri, 16 Feb 2024 at 09:01, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 2/15/2024 9:57 PM, Ulf Hansson wrote: > > On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >> > >> > >> > >> On 2/13/2024 7:21 PM, Ulf Hansson wrote: > >>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote: > >>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote: > >>>>>> > >>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: > >>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: > >>>>>>>> > >>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: > >>>>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org> > >>>>>>>>> > >>>>>>>>> Some power-domains may be capable of relying on the HW to control the power > >>>>>>>>> for a device that's hooked up to it. Typically, for these kinds of > >>>>>>>>> configurations the consumer driver should be able to change the behavior of > >>>>>>>>> power domain at runtime, control the power domain in SW mode for certain > >>>>>>>>> configurations and handover the control to HW mode for other usecases. > >>>>>>>>> > >>>>>>>>> To allow a consumer driver to change the behaviour of the PM domain for its > >>>>>>>>> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, > >>>>>>>>> let's add a corresponding optional genpd callback, ->set_hwmode_dev(), > >>>>>>>>> which the genpd provider should implement if it can support switching > >>>>>>>>> between HW controlled mode and SW controlled mode. Similarly, add the > >>>>>>>>> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and > >>>>>>>>> its corresponding optional genpd callback, ->get_hwmode_dev(), which the > >>>>>>>>> genpd provider can also implement for reading back the mode from the > >>>>>>>>> hardware. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >>>>>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > >>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>>>>>>> --- > >>>>>>>>> drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ > >>>>>>>>> include/linux/pm_domain.h | 17 ++++++++++++ > >>>>>>>>> 2 files changed, 86 insertions(+) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c > >>>>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644 > >>>>>>>>> --- a/drivers/pmdomain/core.c > >>>>>>>>> +++ b/drivers/pmdomain/core.c > >>>>>>>>> @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) > >>>>>>>>> } > >>>>>>>>> EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); > >>>>>>>>> > >>>>>>>>> +/** > >>>>>>>>> + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. > >>>>>>>> > >>>>>>>> This isn't proper kernel-doc > >>>>>>> > >>>>>>> Sorry, I didn't quite get that. What is wrong? > >>>>>>> > >>>>>> > >>>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation > >>>>>> says that there should be () after the function name, and below there > >>>>>> should be a Return: > >>>>> > >>>>> Thanks for the pointers! > >>>>> > >>>>>> > >>>>>>>> > >>>>>>>>> + * > >>>>>>>>> + * @dev: Device for which the HW-mode should be changed. > >>>>>>>>> + * @enable: Value to set or unset the HW-mode. > >>>>>>>>> + * > >>>>>>>>> + * Some PM domains can rely on HW signals to control the power for a device. To > >>>>>>>>> + * allow a consumer driver to switch the behaviour for its device in runtime, > >>>>>>>>> + * which may be beneficial from a latency or energy point of view, this function > >>>>>>>>> + * may be called. > >>>>>>>>> + * > >>>>>>>>> + * It is assumed that the users guarantee that the genpd wouldn't be detached > >>>>>>>>> + * while this routine is getting called. > >>>>>>>>> + * > >>>>>>>>> + * Returns 0 on success and negative error values on failures. > >>>>>>>>> + */ > >>>>>>>>> +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) > >>>>>>>>> +{ > >>>>>>>>> + struct generic_pm_domain *genpd; > >>>>>>>>> + int ret = 0; > >>>>>>>>> + > >>>>>>>>> + genpd = dev_to_genpd_safe(dev); > >>>>>>>>> + if (!genpd) > >>>>>>>>> + return -ENODEV; > >>>>>>>>> + > >>>>>>>>> + if (!genpd->set_hwmode_dev) > >>>>>>>>> + return -EOPNOTSUPP; > >>>>>>>>> + > >>>>>>>>> + genpd_lock(genpd); > >>>>>>>>> + > >>>>>>>>> + if (dev_gpd_data(dev)->hw_mode == enable) > >>>>>>>> > >>>>>>>> Between this and the gdsc patch, the hw_mode state might not match the > >>>>>>>> hardware state at boot. > >>>>>>>> > >>>>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, > >>>>>>>> false) will not bring control to SW - which might be fatal. > >>>>>>> > >>>>>>> Right, good point. > >>>>>>> > >>>>>>> I think we have two ways to deal with this: > >>>>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let > >>>>>>> genpd_add_device() invoke it to synchronize the state. > >>>>>> > >>>>>> I'd suggest that we skip the optimization for now and just let the > >>>>>> update hit the driver on each call. > >>>>> > >>>>> Okay. > >>>>> > >>>>>> > >>>>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call > >>>>>>> ->set_hwmode_dev() to allow an initial state to be set. > >>>>>>> > >>>>>>> The question is then, if we need to allow ->get_hwmode_dev() to be > >>>>>>> optional, if the ->set_hwmode_dev() is supported - or if we can > >>>>>>> require it. What's your thoughts around this? > >>>>>>> > >>>>>> > >>>>>> Iiuc this resource can be shared between multiple clients, and we're > >>>>>> in either case returning the shared state. That would mean a client > >>>>>> acting upon the returned value, is subject to races. > >>>>> > >>>>> Not sure I understand this, but I also don't have in-depth knowledge > >>>>> of how the HW works. > >>>>> > >>>>> Isn't the HW mode set on a per device basis? > >>>>> > >>>>>> > >>>>>> I'm therefore inclined to say that we shouldn't have a getter, other > >>>>>> than for debugging purposes, in which case reading the HW-state or > >>>>>> failing would be reasonable outcomes. > >>>>> > >>>>> If you only want this for debug purposes, it seems better to keep it > >>>>> closer to the rpmh code, rather than adding generic callbacks to the > >>>>> genpd interface. > >>>>> > >>>>> So to conclude, you think having a ->set_hwmode_dev() callback should > >>>>> be sufficient and no caching of the current state? > >>>>> > >>>>> Abel, what's your thoughts around this? > >>>>> > >>>> > >>>> We believe it is good to have get_hwmode_dev() callback supported from > >>>> GenPD, since if multiple devices share a GenPD, and if one device moves > >>>> the GenPD to HW mode, the other device won't be aware of it and second > >>>> device's dev_gpd_data(dev)->hw_mode will still be false. > >>>> > >>>> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign > >>>> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev() > >>>> callback, consumer drivers can use this API to sync the actual HW mode > >>>> of the GenPD. > >>> > >>> Hmm, I thought the HW mode was being set on a per device basis, via > >>> its PM domain. Did I get that wrong? > >>> > >>> Are you saying there could be multiple devices sharing the same PM > >>> domain and thus also sharing the same HW mode? In that case, it sure > >>> sounds like we have synchronization issues to deal with too. > >>> > >> > >> Sorry my bad, currently we don't have usecase where multiple devices > >> sharing the same PM domain that have HW control support, so there is no > >> synchronization issue. > > > > Okay, good! > > > >> > >> But it would be good to have .get_hwmode_dev() callback for consumer > >> drivers to query the actual GenPD mode from HW, whenever they require it. > > > > Okay, no objection from my side. > > > > Then the final question is if we need a variable to keep a cache of > > the current HW mode for each device. Perhaps we should start simple > > and just always invoke the callbacks from genpd, what do you think? > > > > Yes, agree, we can remove the variable and just always invoke the > callbacks from genpd. But we may need the variable to reflect GenPD > mode in debugfs genpd_summary, or need to invoke get callback there as > well to get the current mode. Hmm, after some more thinking I believe it may be best to keep the variable after all. For reasons you point out above. However, we need a way to synchronize the initial HW mode state for a device. Therefore I suggest we invoke the ->get_hwmode_dev() callback from genpd_add_device() and store its return value in the variable. Later the variable can be used for debugfs and returned from dev_pm_genpd_get_hwmode() too. That should work, right? Kind regards Uffe
On 2/28/2024 8:23 PM, Ulf Hansson wrote: > On Fri, 16 Feb 2024 at 09:01, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 2/15/2024 9:57 PM, Ulf Hansson wrote: >>> On Wed, 14 Feb 2024 at 05:29, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 2/13/2024 7:21 PM, Ulf Hansson wrote: >>>>> On Tue, 13 Feb 2024 at 14:10, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 2/2/2024 5:59 PM, Ulf Hansson wrote: >>>>>>> On Fri, 2 Feb 2024 at 00:51, Bjorn Andersson <andersson@kernel.org> wrote: >>>>>>>> >>>>>>>> On Wed, Jan 31, 2024 at 01:12:00PM +0100, Ulf Hansson wrote: >>>>>>>>> On Wed, 31 Jan 2024 at 02:09, Bjorn Andersson <andersson@kernel.org> wrote: >>>>>>>>>> >>>>>>>>>> On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote: >>>>>>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org> >>>>>>>>>>> >>>>>>>>>>> Some power-domains may be capable of relying on the HW to control the power >>>>>>>>>>> for a device that's hooked up to it. Typically, for these kinds of >>>>>>>>>>> configurations the consumer driver should be able to change the behavior of >>>>>>>>>>> power domain at runtime, control the power domain in SW mode for certain >>>>>>>>>>> configurations and handover the control to HW mode for other usecases. >>>>>>>>>>> >>>>>>>>>>> To allow a consumer driver to change the behaviour of the PM domain for its >>>>>>>>>>> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover, >>>>>>>>>>> let's add a corresponding optional genpd callback, ->set_hwmode_dev(), >>>>>>>>>>> which the genpd provider should implement if it can support switching >>>>>>>>>>> between HW controlled mode and SW controlled mode. Similarly, add the >>>>>>>>>>> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and >>>>>>>>>>> its corresponding optional genpd callback, ->get_hwmode_dev(), which the >>>>>>>>>>> genpd provider can also implement for reading back the mode from the >>>>>>>>>>> hardware. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>>>>>>>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>>>>>>>> --- >>>>>>>>>>> drivers/pmdomain/core.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>>>>> include/linux/pm_domain.h | 17 ++++++++++++ >>>>>>>>>>> 2 files changed, 86 insertions(+) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c >>>>>>>>>>> index a1f6cba3ae6c..41b6411d0ef5 100644 >>>>>>>>>>> --- a/drivers/pmdomain/core.c >>>>>>>>>>> +++ b/drivers/pmdomain/core.c >>>>>>>>>>> @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev) >>>>>>>>>>> } >>>>>>>>>>> EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff); >>>>>>>>>>> >>>>>>>>>>> +/** >>>>>>>>>>> + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain. >>>>>>>>>> >>>>>>>>>> This isn't proper kernel-doc >>>>>>>>> >>>>>>>>> Sorry, I didn't quite get that. What is wrong? >>>>>>>>> >>>>>>>> >>>>>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation >>>>>>>> says that there should be () after the function name, and below there >>>>>>>> should be a Return: >>>>>>> >>>>>>> Thanks for the pointers! >>>>>>> >>>>>>>> >>>>>>>>>> >>>>>>>>>>> + * >>>>>>>>>>> + * @dev: Device for which the HW-mode should be changed. >>>>>>>>>>> + * @enable: Value to set or unset the HW-mode. >>>>>>>>>>> + * >>>>>>>>>>> + * Some PM domains can rely on HW signals to control the power for a device. To >>>>>>>>>>> + * allow a consumer driver to switch the behaviour for its device in runtime, >>>>>>>>>>> + * which may be beneficial from a latency or energy point of view, this function >>>>>>>>>>> + * may be called. >>>>>>>>>>> + * >>>>>>>>>>> + * It is assumed that the users guarantee that the genpd wouldn't be detached >>>>>>>>>>> + * while this routine is getting called. >>>>>>>>>>> + * >>>>>>>>>>> + * Returns 0 on success and negative error values on failures. >>>>>>>>>>> + */ >>>>>>>>>>> +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable) >>>>>>>>>>> +{ >>>>>>>>>>> + struct generic_pm_domain *genpd; >>>>>>>>>>> + int ret = 0; >>>>>>>>>>> + >>>>>>>>>>> + genpd = dev_to_genpd_safe(dev); >>>>>>>>>>> + if (!genpd) >>>>>>>>>>> + return -ENODEV; >>>>>>>>>>> + >>>>>>>>>>> + if (!genpd->set_hwmode_dev) >>>>>>>>>>> + return -EOPNOTSUPP; >>>>>>>>>>> + >>>>>>>>>>> + genpd_lock(genpd); >>>>>>>>>>> + >>>>>>>>>>> + if (dev_gpd_data(dev)->hw_mode == enable) >>>>>>>>>> >>>>>>>>>> Between this and the gdsc patch, the hw_mode state might not match the >>>>>>>>>> hardware state at boot. >>>>>>>>>> >>>>>>>>>> With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(, >>>>>>>>>> false) will not bring control to SW - which might be fatal. >>>>>>>>> >>>>>>>>> Right, good point. >>>>>>>>> >>>>>>>>> I think we have two ways to deal with this: >>>>>>>>> 1) If the provider is supporting ->get_hwmode_dev(), we can let >>>>>>>>> genpd_add_device() invoke it to synchronize the state. >>>>>>>> >>>>>>>> I'd suggest that we skip the optimization for now and just let the >>>>>>>> update hit the driver on each call. >>>>>>> >>>>>>> Okay. >>>>>>> >>>>>>>> >>>>>>>>> 2) If the provider doesn't support ->get_hwmode_dev() we need to call >>>>>>>>> ->set_hwmode_dev() to allow an initial state to be set. >>>>>>>>> >>>>>>>>> The question is then, if we need to allow ->get_hwmode_dev() to be >>>>>>>>> optional, if the ->set_hwmode_dev() is supported - or if we can >>>>>>>>> require it. What's your thoughts around this? >>>>>>>>> >>>>>>>> >>>>>>>> Iiuc this resource can be shared between multiple clients, and we're >>>>>>>> in either case returning the shared state. That would mean a client >>>>>>>> acting upon the returned value, is subject to races. >>>>>>> >>>>>>> Not sure I understand this, but I also don't have in-depth knowledge >>>>>>> of how the HW works. >>>>>>> >>>>>>> Isn't the HW mode set on a per device basis? >>>>>>> >>>>>>>> >>>>>>>> I'm therefore inclined to say that we shouldn't have a getter, other >>>>>>>> than for debugging purposes, in which case reading the HW-state or >>>>>>>> failing would be reasonable outcomes. >>>>>>> >>>>>>> If you only want this for debug purposes, it seems better to keep it >>>>>>> closer to the rpmh code, rather than adding generic callbacks to the >>>>>>> genpd interface. >>>>>>> >>>>>>> So to conclude, you think having a ->set_hwmode_dev() callback should >>>>>>> be sufficient and no caching of the current state? >>>>>>> >>>>>>> Abel, what's your thoughts around this? >>>>>>> >>>>>> >>>>>> We believe it is good to have get_hwmode_dev() callback supported from >>>>>> GenPD, since if multiple devices share a GenPD, and if one device moves >>>>>> the GenPD to HW mode, the other device won't be aware of it and second >>>>>> device's dev_gpd_data(dev)->hw_mode will still be false. >>>>>> >>>>>> If we have this dev_pm_genpd_get_hwmode() API supported and if we assign >>>>>> dev_gpd_data(dev)->hw_mode after getting the mode from get_hwmode_dev() >>>>>> callback, consumer drivers can use this API to sync the actual HW mode >>>>>> of the GenPD. >>>>> >>>>> Hmm, I thought the HW mode was being set on a per device basis, via >>>>> its PM domain. Did I get that wrong? >>>>> >>>>> Are you saying there could be multiple devices sharing the same PM >>>>> domain and thus also sharing the same HW mode? In that case, it sure >>>>> sounds like we have synchronization issues to deal with too. >>>>> >>>> >>>> Sorry my bad, currently we don't have usecase where multiple devices >>>> sharing the same PM domain that have HW control support, so there is no >>>> synchronization issue. >>> >>> Okay, good! >>> >>>> >>>> But it would be good to have .get_hwmode_dev() callback for consumer >>>> drivers to query the actual GenPD mode from HW, whenever they require it. >>> >>> Okay, no objection from my side. >>> >>> Then the final question is if we need a variable to keep a cache of >>> the current HW mode for each device. Perhaps we should start simple >>> and just always invoke the callbacks from genpd, what do you think? >>> >> >> Yes, agree, we can remove the variable and just always invoke the >> callbacks from genpd. But we may need the variable to reflect GenPD >> mode in debugfs genpd_summary, or need to invoke get callback there as >> well to get the current mode. > > Hmm, after some more thinking I believe it may be best to keep the > variable after all. For reasons you point out above. > > However, we need a way to synchronize the initial HW mode state for a > device. Therefore I suggest we invoke the ->get_hwmode_dev() callback > from genpd_add_device() and store its return value in the variable. > Later the variable can be used for debugfs and returned from > dev_pm_genpd_get_hwmode() too. > > That should work, right? > Yes, it should work. Thanks, Jagadeesh > Kind regards > Uffe
Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- Changes in v4: - Re-worded 1st patch commit message, as per Bjorn's suggestion, and added Dmitry's R-b tag - Added Bjorn's and Dmitry's R-b tags to the 2nd patch - Re-worded 3rd patch commit message, to better explain the HW_CTRL_TRIGGER flag. - Added mode transition delay when setting mode for GDSC - Added status polling if GDSSC is enabled when transitioning from HW to SW - Re-worded 4th patch commit message to better explain why the HW_CTRL_TRIGGER needs to be used instead - Drop changes to SC7180, SDM845 and SM8550 video CC drivers, as only SC7280 and SM8250 have been tested so far. More platforms (with v6 venus) will be added eventually. - Call genpd set_hwmode API only for v6 and dropped the vcodec_pmdomains_hwctrl. - Re-worded 5th patch commit message accordingly. - Link to v3: https://lore.kernel.org/r/20231101-gdsc-hwctrl-v3-0-0740ae6b2b04@linaro.org/ --- Abel Vesa (1): PM: domains: Add the domain HW-managed mode to the summary Jagadeesh Kona (3): clk: qcom: gdsc: Add set and get hwmode callbacks to switch GDSC mode clk: qcom: Use HW_CTRL_TRIGGER flag to switch video GDSC to HW mode venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode Ulf Hansson (1): PM: domains: Allow devices attached to genpd to be managed by HW drivers/clk/qcom/gdsc.c | 54 +++++++++++++++++ drivers/clk/qcom/gdsc.h | 1 + drivers/clk/qcom/videocc-sc7280.c | 2 +- drivers/clk/qcom/videocc-sm8250.c | 4 +- drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++---- drivers/pmdomain/core.c | 83 +++++++++++++++++++++++++- include/linux/pm_domain.h | 17 ++++++ 7 files changed, 169 insertions(+), 15 deletions(-) --- base-commit: 319fbd8fc6d339e0a1c7b067eed870c518a13a02 change-id: 20231101-gdsc-hwctrl-13f01ea60cbd Best regards,