diff mbox

[v3,2/4] dt-bindings: Add TI SCI PM Domains

Message ID 23936395-d653-56c8-13f9-6dfeb6e9257b@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dave Gerlach Jan. 25, 2017, 4:59 p.m. UTC
On 01/24/2017 04:03 AM, Ulf Hansson wrote:
> On 23 January 2017 at 21:11, Dave Gerlach <d-gerlach@ti.com> 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:


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
>

Comments

Ulf Hansson Jan. 25, 2017, 9:13 p.m. UTC | #1
On 25 January 2017 at 17:59, Dave Gerlach <d-gerlach@ti.com> wrote:
> On 01/24/2017 04:03 AM, Ulf Hansson wrote:
>>
>> On 23 January 2017 at 21:11, Dave Gerlach <d-gerlach@ti.com> 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.

Ok, this seems to work! I am ready to review! :-)

Of course, don't forget to update the existing DT doc for the power
domain bindings.

Kind regards
Uffe
Rob Herring (Arm) Jan. 25, 2017, 10:32 p.m. UTC | #2
On Wed, Jan 25, 2017 at 10:59 AM, Dave Gerlach <d-gerlach@ti.com> wrote:
> On 01/24/2017 04:03 AM, Ulf Hansson wrote:
>>
>> On 23 January 2017 at 21:11, Dave Gerlach <d-gerlach@ti.com> 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:

I'd still like to understand how the ID is used in order to understand
if as a power-domain cell is appropriate. I think the test is this: is
the ID meaningful to (or defined by) the device or the power domain
controller? The former should be a property in the device node. The
latter should be phandle args.

> 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);

This you should not be doing. The client driver shouldn't care how
many cells or what their values are.

Rob
Dave Gerlach Jan. 26, 2017, 3:09 p.m. UTC | #3
On 01/25/2017 04:32 PM, Rob Herring wrote:
> On Wed, Jan 25, 2017 at 10:59 AM, Dave Gerlach <d-gerlach@ti.com> wrote:
>> On 01/24/2017 04:03 AM, Ulf Hansson wrote:
>>>
>>> On 23 January 2017 at 21:11, Dave Gerlach <d-gerlach@ti.com> 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:
>
> I'd still like to understand how the ID is used in order to understand
> if as a power-domain cell is appropriate. I think the test is this: is
> the ID meaningful to (or defined by) the device or the power domain
> controller? The former should be a property in the device node. The
> latter should be phandle args.

The device itself doesn't care about the ID or ever need to know what it is. The 
power domain controller is the only user of the ID. The power domain controller 
needs the ID for a specific device to be able to turn it on, so during probe of 
each device the power-domain controller makes this association which is why I 
included it in each device node, so I think it fits as a phandle arg based on 
your test.

>
>> 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);
>
> This you should not be doing. The client driver shouldn't care how
> many cells or what their values are.

Client drivers in other places use xlate functions, like in the drivers/reset 
framework, to interpret the cells. Is doing it this way really that different?

Regards,
Dave

>
> Rob
>
Rob Herring (Arm) Jan. 26, 2017, 4 p.m. UTC | #4
On Thu, Jan 26, 2017 at 9:09 AM, Dave Gerlach <d-gerlach@ti.com> wrote:
> On 01/25/2017 04:32 PM, Rob Herring wrote:
>>
>> On Wed, Jan 25, 2017 at 10:59 AM, Dave Gerlach <d-gerlach@ti.com> wrote:

[...]

>>> 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);
>>
>>
>> This you should not be doing. The client driver shouldn't care how
>> many cells or what their values are.
>
>
> Client drivers in other places use xlate functions, like in the
> drivers/reset framework, to interpret the cells. Is doing it this way really
> that different?

Oh, I was thinking the client driver, not the power domain controller
driver. NM.

Rob
diff mbox

Patch

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;
  }