Received-SPF: pass (google.com: domain of linux-kernel+bounces-46404-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240122-gdsc-hwctrl-v4-0-9061e8a7aa07@linaro.org>
 <20240122-gdsc-hwctrl-v4-1-9061e8a7aa07@linaro.org> <tax3c6o5qjegy6tv3zbgrd5rencfvypr3zg7twxfrmdngscp74@n44ei3q63g64>
In-Reply-To: <tax3c6o5qjegy6tv3zbgrd5rencfvypr3zg7twxfrmdngscp74@n44ei3q63g64>
From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Wed, 31 Jan 2024 13:12:00 +0100
Message-ID: <CAPDyKFpdtrWbzNksLoY++aOY7Ltyt1HhtLZo8bj8sQ05-4Sq0g@mail.gmail.com>
Subject: Re: [PATCH v4 1/5] PM: domains: Allow devices attached to genpd to be
 managed by HW
To: Bjorn Andersson <andersson@kernel.org>, Abel Vesa <abel.vesa@linaro.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Kevin Hilman <khilman@kernel.org>, Pavel Machek <pavel@ucw.cz>, 
	Len Brown <len.brown@intel.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	Andy Gross <agross@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, 
	Stanimir Varbanov <stanimir.k.varbanov@gmail.com>, Vikash Garodia <quic_vgarodia@quicinc.com>, 
	"Bryan O'Donoghue" <bryan.odonoghue@linaro.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, 
	Taniya Das <quic_tdas@quicinc.com>, Jagadeesh Kona <quic_jkona@quicinc.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, linux-pm@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, 
	linux-clk@vger.kernel.org, linux-media@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

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