Signed-off-by: Abel Vesa <[email protected]>
---
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/[email protected]/
---
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,
--
Abel Vesa <[email protected]>
Now that genpd supports dynamically switching the control for an
attached device between hardware- and software-mode, let's add this
information to the genpd summary in debugfs.
Suggested-by: Taniya Das <[email protected]>
Signed-off-by: Abel Vesa <[email protected]>
Reviewed-by: Ulf Hansson <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
drivers/pmdomain/core.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 41b6411d0ef5..841fde9e9f87 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -3147,6 +3147,15 @@ static void rtpm_status_str(struct seq_file *s, struct device *dev)
seq_printf(s, "%-25s ", p);
}
+static void mode_status_str(struct seq_file *s, struct device *dev)
+{
+ struct generic_pm_domain_data *gpd_data;
+
+ gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+
+ seq_printf(s, "%20s", gpd_data->hw_mode ? "HW" : "SW");
+}
+
static void perf_status_str(struct seq_file *s, struct device *dev)
{
struct generic_pm_domain_data *gpd_data;
@@ -3205,6 +3214,7 @@ static int genpd_summary_one(struct seq_file *s,
seq_printf(s, "\n %-50s ", kobj_path);
rtpm_status_str(s, pm_data->dev);
perf_status_str(s, pm_data->dev);
+ mode_status_str(s, pm_data->dev);
kfree(kobj_path);
}
@@ -3221,8 +3231,8 @@ static int summary_show(struct seq_file *s, void *data)
int ret = 0;
seq_puts(s, "domain status children performance\n");
- seq_puts(s, " /device runtime status\n");
- seq_puts(s, "----------------------------------------------------------------------------------------------\n");
+ seq_puts(s, " /device runtime status managed by\n");
+ seq_puts(s, "------------------------------------------------------------------------------------------------------------\n");
ret = mutex_lock_interruptible(&gpd_list_lock);
if (ret)
--
2.34.1
From: Jagadeesh Kona <[email protected]>
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.
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.
Signed-off-by: Jagadeesh Kona <[email protected]>
Signed-off-by: Abel Vesa <[email protected]>
---
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)) {
+ 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.
+ */
+ if (!(val & SW_COLLAPSE_MASK))
+ 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;
+}
+
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
From: Jagadeesh Kona <[email protected]>
The current HW_CTRL flag switches the video GDSC to HW control mode as
part of GDSC enable itself, and HW can disable the GDSC right after the
GDSC is moved to HW mode. This can lead to subsequent video clock enable
failures that are dependent on the GDSC if GDSC is collapsed by HW. Also,
while few video hardware registers are being programmed by TZ, GDSC
should be in non collapsed state which can be achieved by keeping
it in SW mode.
Use HW_CTRL_TRIGGER flag instead which allows consumer drivers to switch
the video GDSC to HW/SW control modes at runtime using
dev_pm_genpd_set_hwmode API as per the requirement.
Signed-off-by: Jagadeesh Kona <[email protected]>
Signed-off-by: Abel Vesa <[email protected]>
---
drivers/clk/qcom/videocc-sc7280.c | 2 +-
drivers/clk/qcom/videocc-sm8250.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c
index 615695d82319..3d07b1e95986 100644
--- a/drivers/clk/qcom/videocc-sc7280.c
+++ b/drivers/clk/qcom/videocc-sc7280.c
@@ -236,7 +236,7 @@ static struct gdsc mvs0_gdsc = {
.name = "mvs0_gdsc",
},
.pwrsts = PWRSTS_OFF_ON,
- .flags = HW_CTRL | RETAIN_FF_ENABLE,
+ .flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE,
};
static struct gdsc mvsc_gdsc = {
diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
index ad46c4014a40..c1b73d852f1c 100644
--- a/drivers/clk/qcom/videocc-sm8250.c
+++ b/drivers/clk/qcom/videocc-sm8250.c
@@ -293,7 +293,7 @@ static struct gdsc mvs0_gdsc = {
.pd = {
.name = "mvs0_gdsc",
},
- .flags = HW_CTRL,
+ .flags = HW_CTRL_TRIGGER,
.pwrsts = PWRSTS_OFF_ON,
};
@@ -302,7 +302,7 @@ static struct gdsc mvs1_gdsc = {
.pd = {
.name = "mvs1_gdsc",
},
- .flags = HW_CTRL,
+ .flags = HW_CTRL_TRIGGER,
.pwrsts = PWRSTS_OFF_ON,
};
--
2.34.1
From: Jagadeesh Kona <[email protected]>
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.
With newer implementation, the mode of vcodec gdsc gets switched only in
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.
Signed-off-by: Jagadeesh Kona <[email protected]>
Signed-off-by: Abel Vesa <[email protected]>
---
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);
+ 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 Mon, 22 Jan 2024 at 09:47, Abel Vesa <[email protected]> wrote:
>
> Signed-off-by: Abel Vesa <[email protected]>
> ---
> 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/[email protected]/
>
> ---
> 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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Abel Vesa <[email protected]>
> ---
> 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 <[email protected]>
> >
> > 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:05AM +0200, Abel Vesa wrote:
> From: Jagadeesh Kona <[email protected]>
>
> 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"?
> 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"?
How come? Today this GDSC is operating in HW control mode, before,
during and after the clock operation.
> Signed-off-by: Jagadeesh Kona <[email protected]>
> Signed-off-by: Abel Vesa <[email protected]>
> ---
> 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...
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.
PS. I trust there's no case whre legacy_binding = true, or that that
code path does not need similar workaround?
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 4:30 AM, Bjorn Andersson wrote:
> On Mon, Jan 22, 2024 at 10:47:03AM +0200, Abel Vesa wrote:
>> From: Jagadeesh Kona <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Abel Vesa <[email protected]>
>> ---
>> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Abel Vesa <[email protected]>
>> ---
>> 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 <[email protected]>
>>>
>>> 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