On 01/24/2017 04:03 AM, Ulf Hansson wrote:
On 23 January 2017 at 21:11, Dave Gerlach <d-gerlach@xxxxxx> wrote:
On 01/20/2017 10:52 AM, Ulf Hansson wrote:
[...]
Another option is create something new either common or TI SCI
specific. It could be just a table of ids and phandles in the SCI
node. I'm much more comfortable with an isolated property in one node
than something scattered throughout the DT.
To me, this seems like the best possible solution.
However, perhaps we should also consider the SCPI Generic power domain
(drivers/firmware/scpi_pm_domain.c), because I believe it's closely
related.
To change the power state of a device, this PM domain calls
scpi_device_set|get_power_state() (drivers/firmware/arm_scpi.c), which
also needs a device id as a parameter. Very similar to our case with
the TI SCI domain.
Currently these SCPI device ids lacks corresponding DT bindings, so
the scpi_pm_domain tries to work around it by assigning ids
dynamically at genpd creation time.
That makes me wonder, whether we should think of something
common/generic?
When you say something common/generic, do you mean a better binding for
genpd,
or something bigger than that like a new driver? Because I do think a
phandle
cell left open for the genpd provider to interpret solves both the scpi
and
ti-sci problem we are facing here in the best way. Using generic PM
domains lets
us do exactly what we want apart from interpreting the phandle cell with
our
driver, and I feel like anything else we try at this point is just going
to be
to work around that. Is bringing back genpd xlate something we can
discuss?
Bringing back xlate, how would that help? Wouldn't that just mean that
you will get one genpd per device? That's not an option, I think we
are all in agreement to that.
Sure, perhaps the custom xlate wouldn't be the right way to do it, as we
wouldn't be able to associate a device directly to a phandle, at least with
how it was implemented before, but I think we can skip that entirely. Does
opening up the interpretation of the cells of the 'power-domains' phandle
not solve all of these issues? Is that out of the question?
genpd_xlate_simple currently just makes sure the args_count of the
'power-domains' phandle was zero and bails if it was not. Why couldn't we
remove this check and let the driver interpret it while still using
of_genpd_add_provider_simple to register the provider? It's still a 'simple'
provider from the perspective of the genpd framework and the actual pm
domain mapping will not change, but now the driver can parse the cells and
do whatever it needs to, such as reading a device id.
I think that's a bit more flexible and will avoid breaking anything that is
there today.
Would you mind providing an example? Perhaps also some code snippets
dealing with the parsing?
So again the goal of this is to move the ti,sci-id value back to
power-domains phandle instead of having a separate property, so that
would be step one in the DT. Then in the power-domains node change
#power-domain-cells to one. And then from there, the only change to the
genpd framework is this:
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a5e1262b964b..b82e61f0bcfa 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1603,8 +1603,6 @@ static struct generic_pm_domain genpd_xlate_simple
struct of_phandle_args *genpdspec,
void *data)
{
- if (genpdspec->args_count != 0)
- return ERR_PTR(-EINVAL);
return data;
}
because genpd_xlate_simple only checks that the phandle is zero so that
it can fail if it is not, but there's no functional reason it needs to
do this. The genpd framework works as it did before no matter what the
cells are set to if using of_genpd_add_provider_simple. Then in the
attach_dev callback inside the ti_sci_pm_domains driver instead of doing
ret = of_property_read_u32(np, "ti,sci-id", &idx);
to read the ti,sc-id for a device into idx we can now do:
ret = of_parse_phandle_with_args(np, "power-domains",
"#power-domain-cells", 0, &pd_args);
idx = pd_args.args[0];
or even simpler from within our driver
ret = of_property_read_u32_index(np, "power-domains", 1, &idx);
To read the value into idx.
This requires minimal changes to the genpd framework and gives the
option for the driver to interpret the cell manually when using a simple
provider. The genpd framework still uses the phandle just to get the
power-domain device and the cells are left entirely up to the driver to
interpret, but if desired you could still use the genpd onecell driver
for a specific use of the phandle cell, or use it with zero cells.
I can send out an updated series if there are no major objections, or
just to start discussion there.
Regards,
Dave
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
