diff mbox

[6/9] ARM: tegra: Export tegra_powergate_power_on

Message ID 1401881559-18469-7-git-send-email-mperttunen@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mikko Perttunen June 4, 2014, 11:32 a.m. UTC
This symbol needs to be exported to power on rails without using
tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
cannot be used in situations where the driver wants to handle clocking
by itself.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm/mach-tegra/powergate.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Stephen Warren June 16, 2014, 10:01 p.m. UTC | #1
On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> This symbol needs to be exported to power on rails without using
> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> cannot be used in situations where the driver wants to handle clocking
> by itself.

Thierry, are you OK with this change?
Thierry Reding June 17, 2014, 12:13 p.m. UTC | #2
On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > This symbol needs to be exported to power on rails without using
> > tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > cannot be used in situations where the driver wants to handle clocking
> > by itself.
> 
> Thierry, are you OK with this change?

I would've preferred tegra_powergate_sequence_power_up() to be used
consistently in all drivers. I'm still not convinced that using the
platform AHCI driver this way is really the best option, since we're
bending over backwards to fit into what this driver dictates. There
shouldn't be a need for that.

Thierry
Mikko Perttunen June 17, 2014, 12:28 p.m. UTC | #3
I'll try removing use of all libahci_platform stuff except 
ahci_platform_init_host for v2.

On 17/06/14 15:13, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>>> This symbol needs to be exported to power on rails without using
>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
>>> cannot be used in situations where the driver wants to handle clocking
>>> by itself.
>>
>> Thierry, are you OK with this change?
>
> I would've preferred tegra_powergate_sequence_power_up() to be used
> consistently in all drivers. I'm still not convinced that using the
> platform AHCI driver this way is really the best option, since we're
> bending over backwards to fit into what this driver dictates. There
> shouldn't be a need for that.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>
Peter De Schrijver June 17, 2014, 2:01 p.m. UTC | #4
On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > > This symbol needs to be exported to power on rails without using
> > > tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > > cannot be used in situations where the driver wants to handle clocking
> > > by itself.
> > 
> > Thierry, are you OK with this change?
> 
> I would've preferred tegra_powergate_sequence_power_up() to be used

I don't think the current tegra_powergate_sequence_power_up() API is very well
defined though. I don't think the clocks and resets required by the sequence
should be provided by the driver. For one, there can be several clocks and
resets that need to be controlled for a single domain.

Cheers,

Peter.
Thierry Reding June 17, 2014, 9:51 p.m. UTC | #5
On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > > On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > > > This symbol needs to be exported to power on rails without using
> > > > tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > > > cannot be used in situations where the driver wants to handle clocking
> > > > by itself.
> > > 
> > > Thierry, are you OK with this change?
> > 
> > I would've preferred tegra_powergate_sequence_power_up() to be used
> 
> I don't think the current tegra_powergate_sequence_power_up() API is very well
> defined though. I don't think the clocks and resets required by the sequence
> should be provided by the driver. For one, there can be several clocks and
> resets that need to be controlled for a single domain.

Do you have any suggestions for what the API should look like? Even if
we plan to move to some different API, I think there's some advantage in
using it consistently if for no other reason than to make it easier to
replace occurrences later on.

Thierry
Peter De Schrijver June 18, 2014, 12:18 p.m. UTC | #6
On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> > On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > > 
> > > On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > > > On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > > > > This symbol needs to be exported to power on rails without using
> > > > > tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > > > > cannot be used in situations where the driver wants to handle clocking
> > > > > by itself.
> > > > 
> > > > Thierry, are you OK with this change?
> > > 
> > > I would've preferred tegra_powergate_sequence_power_up() to be used
> > 
> > I don't think the current tegra_powergate_sequence_power_up() API is very well
> > defined though. I don't think the clocks and resets required by the sequence
> > should be provided by the driver. For one, there can be several clocks and
> > resets that need to be controlled for a single domain.
> 
> Do you have any suggestions for what the API should look like? Even if
> we plan to move to some different API, I think there's some advantage in
> using it consistently if for no other reason than to make it easier to
> replace occurrences later on.
> 

I think the API should only have the domain ID as input so:

int tegra_powerdomain_on(int id) 

/*
 * Prerequisites: domain is off
 * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
 */

int tegra_powerdomain_off(int id)

/*
 * Prerequisites: all clocks of the modules in the domain are off
 * result: domain is off
 */

Cheers,

Peter/
Stephen Warren June 18, 2014, 3:37 p.m. UTC | #7
On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
>>>>> Old Signed by an unknown key
>>>>
>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>>>>>> This symbol needs to be exported to power on rails without using
>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
>>>>>> cannot be used in situations where the driver wants to handle clocking
>>>>>> by itself.
>>>>>
>>>>> Thierry, are you OK with this change?
>>>>
>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
>>>
>>> I don't think the current tegra_powergate_sequence_power_up() API is very well
>>> defined though. I don't think the clocks and resets required by the sequence
>>> should be provided by the driver. For one, there can be several clocks and
>>> resets that need to be controlled for a single domain.
>>
>> Do you have any suggestions for what the API should look like? Even if
>> we plan to move to some different API, I think there's some advantage in
>> using it consistently if for no other reason than to make it easier to
>> replace occurrences later on.
>>
> 
> I think the API should only have the domain ID as input so:
> 
> int tegra_powerdomain_on(int id) 
> 
> /*
>  * Prerequisites: domain is off
>  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
>  */
> 
> int tegra_powerdomain_off(int id)
> 
> /*
>  * Prerequisites: all clocks of the modules in the domain are off
>  * result: domain is off
>  */

That doesn't make sense; the PMC doesn't have access to the clock and
reset IDs - that's why the API requires them to be passed in.
Peter De Schrijver June 19, 2014, 8:02 a.m. UTC | #8
On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> > On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> >> * PGP Signed by an unknown key
> >>
> >> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> >>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> >>>>> Old Signed by an unknown key
> >>>>
> >>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> >>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> >>>>>> This symbol needs to be exported to power on rails without using
> >>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> >>>>>> cannot be used in situations where the driver wants to handle clocking
> >>>>>> by itself.
> >>>>>
> >>>>> Thierry, are you OK with this change?
> >>>>
> >>>> I would've preferred tegra_powergate_sequence_power_up() to be used
> >>>
> >>> I don't think the current tegra_powergate_sequence_power_up() API is very well
> >>> defined though. I don't think the clocks and resets required by the sequence
> >>> should be provided by the driver. For one, there can be several clocks and
> >>> resets that need to be controlled for a single domain.
> >>
> >> Do you have any suggestions for what the API should look like? Even if
> >> we plan to move to some different API, I think there's some advantage in
> >> using it consistently if for no other reason than to make it easier to
> >> replace occurrences later on.
> >>
> > 
> > I think the API should only have the domain ID as input so:
> > 
> > int tegra_powerdomain_on(int id) 
> > 
> > /*
> >  * Prerequisites: domain is off
> >  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
> >  */
> > 
> > int tegra_powerdomain_off(int id)
> > 
> > /*
> >  * Prerequisites: all clocks of the modules in the domain are off
> >  * result: domain is off
> >  */
> 
> That doesn't make sense; the PMC doesn't have access to the clock and
> reset IDs - that's why the API requires them to be passed in.
> 

We should make driver look them up by name then. It doesn't make sense to
move this knowledge to the drivers as there can be several modules in 1
powerdomain. So every driver would then need to have access to all clock
and reset IDs of the modules of the domain?

Cheers,

Peter.
Peter De Schrijver June 19, 2014, 8:36 a.m. UTC | #9
On Thu, Jun 19, 2014 at 10:02:35AM +0200, Peter De Schrijver wrote:
> On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
> > On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> > > On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> > >> * PGP Signed by an unknown key
> > >>
> > >> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> > >>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> > >>>>> Old Signed by an unknown key
> > >>>>
> > >>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > >>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > >>>>>> This symbol needs to be exported to power on rails without using
> > >>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > >>>>>> cannot be used in situations where the driver wants to handle clocking
> > >>>>>> by itself.
> > >>>>>
> > >>>>> Thierry, are you OK with this change?
> > >>>>
> > >>>> I would've preferred tegra_powergate_sequence_power_up() to be used
> > >>>
> > >>> I don't think the current tegra_powergate_sequence_power_up() API is very well
> > >>> defined though. I don't think the clocks and resets required by the sequence
> > >>> should be provided by the driver. For one, there can be several clocks and
> > >>> resets that need to be controlled for a single domain.
> > >>
> > >> Do you have any suggestions for what the API should look like? Even if
> > >> we plan to move to some different API, I think there's some advantage in
> > >> using it consistently if for no other reason than to make it easier to
> > >> replace occurrences later on.
> > >>
> > > 
> > > I think the API should only have the domain ID as input so:
> > > 
> > > int tegra_powerdomain_on(int id) 
> > > 
> > > /*
> > >  * Prerequisites: domain is off
> > >  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
> > >  */
> > > 
> > > int tegra_powerdomain_off(int id)
> > > 
> > > /*
> > >  * Prerequisites: all clocks of the modules in the domain are off
> > >  * result: domain is off
> > >  */
> > 
> > That doesn't make sense; the PMC doesn't have access to the clock and
> > reset IDs - that's why the API requires them to be passed in.
> > 
> 
> We should make driver look them up by name then. It doesn't make sense to

I mean the PMC driver here.
Stephen Warren June 19, 2014, 4:01 p.m. UTC | #10
On 06/19/2014 02:02 AM, Peter De Schrijver wrote:
> On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
>> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
>>> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
>>>> * PGP Signed by an unknown key
>>>>
>>>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
>>>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
>>>>>>> Old Signed by an unknown key
>>>>>>
>>>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
>>>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
>>>>>>>> This symbol needs to be exported to power on rails without using
>>>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
>>>>>>>> cannot be used in situations where the driver wants to handle clocking
>>>>>>>> by itself.
>>>>>>>
>>>>>>> Thierry, are you OK with this change?
>>>>>>
>>>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
>>>>>
>>>>> I don't think the current tegra_powergate_sequence_power_up() API is very well
>>>>> defined though. I don't think the clocks and resets required by the sequence
>>>>> should be provided by the driver. For one, there can be several clocks and
>>>>> resets that need to be controlled for a single domain.
>>>>
>>>> Do you have any suggestions for what the API should look like? Even if
>>>> we plan to move to some different API, I think there's some advantage in
>>>> using it consistently if for no other reason than to make it easier to
>>>> replace occurrences later on.
>>>>
>>>
>>> I think the API should only have the domain ID as input so:
>>>
>>> int tegra_powerdomain_on(int id) 
>>>
>>> /*
>>>  * Prerequisites: domain is off
>>>  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
>>>  */
>>>
>>> int tegra_powerdomain_off(int id)
>>>
>>> /*
>>>  * Prerequisites: all clocks of the modules in the domain are off
>>>  * result: domain is off
>>>  */
>>
>> That doesn't make sense; the PMC doesn't have access to the clock and
>> reset IDs - that's why the API requires them to be passed in.
>>
> 
> We should make driver look them up by name then. It doesn't make sense to
> move this knowledge to the drivers as there can be several modules in 1
> powerdomain. So every driver would then need to have access to all clock
> and reset IDs of the modules of the domain?

Having the drivers know the clocks, resets, and power domains that
affect their HW module seems perfectly reasonable.

Do we actually have any case where unrelated modules are in the same
power domain /and/ there's some need for those drivers to manipulate the
power domain?

BTW, any objections here should have been raised when the initial API
design for powergating was created. Now that we have the current API,
which in turn has driven the DT bindings for at least some HW modules,
we're stuck with it due to DT ABIness. Well, perhaps we can introduce
something new, but we'll always have to support the old way, so there's
little point.
Peter De Schrijver June 23, 2014, 10:14 a.m. UTC | #11
On Thu, Jun 19, 2014 at 06:01:47PM +0200, Stephen Warren wrote:
> On 06/19/2014 02:02 AM, Peter De Schrijver wrote:
> > On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
> >> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> >>> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> >>>> * PGP Signed by an unknown key
> >>>>
> >>>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> >>>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> >>>>>>> Old Signed by an unknown key
> >>>>>>
> >>>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> >>>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> >>>>>>>> This symbol needs to be exported to power on rails without using
> >>>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> >>>>>>>> cannot be used in situations where the driver wants to handle clocking
> >>>>>>>> by itself.
> >>>>>>>
> >>>>>>> Thierry, are you OK with this change?
> >>>>>>
> >>>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
> >>>>>
> >>>>> I don't think the current tegra_powergate_sequence_power_up() API is very well
> >>>>> defined though. I don't think the clocks and resets required by the sequence
> >>>>> should be provided by the driver. For one, there can be several clocks and
> >>>>> resets that need to be controlled for a single domain.
> >>>>
> >>>> Do you have any suggestions for what the API should look like? Even if
> >>>> we plan to move to some different API, I think there's some advantage in
> >>>> using it consistently if for no other reason than to make it easier to
> >>>> replace occurrences later on.
> >>>>
> >>>
> >>> I think the API should only have the domain ID as input so:
> >>>
> >>> int tegra_powerdomain_on(int id) 
> >>>
> >>> /*
> >>>  * Prerequisites: domain is off
> >>>  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
> >>>  */
> >>>
> >>> int tegra_powerdomain_off(int id)
> >>>
> >>> /*
> >>>  * Prerequisites: all clocks of the modules in the domain are off
> >>>  * result: domain is off
> >>>  */
> >>
> >> That doesn't make sense; the PMC doesn't have access to the clock and
> >> reset IDs - that's why the API requires them to be passed in.
> >>
> > 
> > We should make driver look them up by name then. It doesn't make sense to
> > move this knowledge to the drivers as there can be several modules in 1
> > powerdomain. So every driver would then need to have access to all clock
> > and reset IDs of the modules of the domain?
> 
> Having the drivers know the clocks, resets, and power domains that
> affect their HW module seems perfectly reasonable.
> 

Yes, but the problem is that you also need clocks and reset of other modules
in the same domain to safely control the domain's status. Eg: the ISPs, VI and
CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
public documentation. So it's not unlikely a VI and CSI driver will upstreamed
someday which means we would need to control the domain and therefore would
need to tell that driver about the ISPs clocks and resets even though the
driver doesn't know anything about the ISP hw otherwise.

> Do we actually have any case where unrelated modules are in the same
> power domain /and/ there's some need for those drivers to manipulate the
> power domain?
> 

According to the TRM, the reset state of the power domains is undefined. While
that seems unlikely, I do think the kernel should not assume any domain is on
apart from the obvious ones (like the CPU domain it is running on),

> BTW, any objections here should have been raised when the initial API
> design for powergating was created. Now that we have the current API,
> which in turn has driven the DT bindings for at least some HW modules,
> we're stuck with it due to DT ABIness. Well, perhaps we can introduce
> something new, but we'll always have to support the old way, so there's
> little point.

I don't think the bindings need to be changed at all as long as the PMC driver
can parse the relevant DT nodes to get the clock and reset information.

Cheers,

Peter.
Thierry Reding July 8, 2014, 1:05 p.m. UTC | #12
On Mon, Jun 23, 2014 at 01:14:42PM +0300, Peter De Schrijver wrote:
> On Thu, Jun 19, 2014 at 06:01:47PM +0200, Stephen Warren wrote:
> > On 06/19/2014 02:02 AM, Peter De Schrijver wrote:
> > > On Wed, Jun 18, 2014 at 05:37:54PM +0200, Stephen Warren wrote:
> > >> On 06/18/2014 06:18 AM, Peter De Schrijver wrote:
> > >>> On Tue, Jun 17, 2014 at 11:51:20PM +0200, Thierry Reding wrote:
> > >>>> * PGP Signed by an unknown key
> > >>>>
> > >>>> On Tue, Jun 17, 2014 at 05:01:46PM +0300, Peter De Schrijver wrote:
> > >>>>> On Tue, Jun 17, 2014 at 02:13:15PM +0200, Thierry Reding wrote:
> > >>>>>>> Old Signed by an unknown key
> > >>>>>>
> > >>>>>> On Mon, Jun 16, 2014 at 04:01:02PM -0600, Stephen Warren wrote:
> > >>>>>>> On 06/04/2014 05:32 AM, Mikko Perttunen wrote:
> > >>>>>>>> This symbol needs to be exported to power on rails without using
> > >>>>>>>> tegra_powergate_sequence_power_up. tegra_powergate_sequence_power_up
> > >>>>>>>> cannot be used in situations where the driver wants to handle clocking
> > >>>>>>>> by itself.
> > >>>>>>>
> > >>>>>>> Thierry, are you OK with this change?
> > >>>>>>
> > >>>>>> I would've preferred tegra_powergate_sequence_power_up() to be used
> > >>>>>
> > >>>>> I don't think the current tegra_powergate_sequence_power_up() API is very well
> > >>>>> defined though. I don't think the clocks and resets required by the sequence
> > >>>>> should be provided by the driver. For one, there can be several clocks and
> > >>>>> resets that need to be controlled for a single domain.
> > >>>>
> > >>>> Do you have any suggestions for what the API should look like? Even if
> > >>>> we plan to move to some different API, I think there's some advantage in
> > >>>> using it consistently if for no other reason than to make it easier to
> > >>>> replace occurrences later on.
> > >>>>
> > >>>
> > >>> I think the API should only have the domain ID as input so:
> > >>>
> > >>> int tegra_powerdomain_on(int id) 
> > >>>
> > >>> /*
> > >>>  * Prerequisites: domain is off
> > >>>  * Result: domain is on, clocks of the modules in the domain are off, modules are in reset
> > >>>  */
> > >>>
> > >>> int tegra_powerdomain_off(int id)
> > >>>
> > >>> /*
> > >>>  * Prerequisites: all clocks of the modules in the domain are off
> > >>>  * result: domain is off
> > >>>  */
> > >>
> > >> That doesn't make sense; the PMC doesn't have access to the clock and
> > >> reset IDs - that's why the API requires them to be passed in.
> > >>
> > > 
> > > We should make driver look them up by name then. It doesn't make sense to
> > > move this knowledge to the drivers as there can be several modules in 1
> > > powerdomain. So every driver would then need to have access to all clock
> > > and reset IDs of the modules of the domain?
> > 
> > Having the drivers know the clocks, resets, and power domains that
> > affect their HW module seems perfectly reasonable.
> > 
> 
> Yes, but the problem is that you also need clocks and reset of other modules
> in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> someday which means we would need to control the domain and therefore would
> need to tell that driver about the ISPs clocks and resets even though the
> driver doesn't know anything about the ISP hw otherwise.

Can't we make powergates reference counted so that they don't get
disabled as long as there are any users? Looking for example at the
display controller driver, modules don't seem to care overly much about
the powergate's state except that it needs to be turned on before they
touch some of the registers.

From a bit of experimentation it also seems like the sequence encoded
within tegra_powergate_sequence_power_up() isn't at all necessary. I
couldn't find an authoritative reference for that either, so I'm tempted
to conclude that it was simply cargo-culted from the dark-ages.

So I'm thinking that if we ever move to use power domains for this, we
may be able to just drop any extra handling (well, we'd need to keep it
for backwards-compatibility... *sigh*) and let drivers handle the clock
and reset resources.

On the other hand, given that we already need to keep the existing code
for backwards-compatibility, I'm not sure there's a real advantage in
turning them into power domains, since we'd be adding extra code without
an clear gains (especially since it seems like we'd need even more code
to properly handle suspend/resume in drivers that need powergates).

> > Do we actually have any case where unrelated modules are in the same
> > power domain /and/ there's some need for those drivers to manipulate the
> > power domain?
> > 
> 
> According to the TRM, the reset state of the power domains is undefined. While
> that seems unlikely, I do think the kernel should not assume any domain is on
> apart from the obvious ones (like the CPU domain it is running on),

Hm... I thought I had seen some documents specifically giving the reset
states of the power partitions. Perhaps it wasn't in the TRM, though.
But I agree that drivers generally shouldn't be assuming anything about
the power partition states.

Thierry
Peter De Schrijver July 8, 2014, 2:11 p.m. UTC | #13
> > 
> > Yes, but the problem is that you also need clocks and reset of other modules
> > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > someday which means we would need to control the domain and therefore would
> > need to tell that driver about the ISPs clocks and resets even though the
> > driver doesn't know anything about the ISP hw otherwise.
> 
> Can't we make powergates reference counted so that they don't get
> disabled as long as there are any users? Looking for example at the

We could, but then why not switch to the powerdomain code and make powering
off a domain a NOP until we sorted out the context save/restore or fixed
the framework to allow for suspend without turning off the domains?

> display controller driver, modules don't seem to care overly much about
> the powergate's state except that it needs to be turned on before they
> touch some of the registers.
> 
> From a bit of experimentation it also seems like the sequence encoded
> within tegra_powergate_sequence_power_up() isn't at all necessary. I
> couldn't find an authoritative reference for that either, so I'm tempted
> to conclude that it was simply cargo-culted from the dark-ages.
> 
> So I'm thinking that if we ever move to use power domains for this, we
> may be able to just drop any extra handling (well, we'd need to keep it
> for backwards-compatibility... *sigh*) and let drivers handle the clock
> and reset resources.
> 
> On the other hand, given that we already need to keep the existing code
> for backwards-compatibility, I'm not sure there's a real advantage in
> turning them into power domains, since we'd be adding extra code without
> an clear gains (especially since it seems like we'd need even more code
> to properly handle suspend/resume in drivers that need powergates).
> 

Unless we fix the framework to require context save/restore for suspend.
There is a good reason to do that. context save/restore requires energy
as well, so it's not a given that turning off domains in system suspend
will save power.

Cheers,

Peter.
Thierry Reding July 9, 2014, 6:31 a.m. UTC | #14
On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > 
> > > Yes, but the problem is that you also need clocks and reset of other modules
> > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > someday which means we would need to control the domain and therefore would
> > > need to tell that driver about the ISPs clocks and resets even though the
> > > driver doesn't know anything about the ISP hw otherwise.
> > 
> > Can't we make powergates reference counted so that they don't get
> > disabled as long as there are any users? Looking for example at the
> 
> We could, but then why not switch to the powerdomain code and make powering
> off a domain a NOP until we sorted out the context save/restore or fixed
> the framework to allow for suspend without turning off the domains?

Well, one of the reasons why I'm not sure it's worth the effort at this
point is that we can't get rid of the tegra_powergate_*() API anyway
because of backwards compatibility. So we're going to add code (without
getting rid of old code) merely to support some generic framework. That
doesn't sound very useful to me.

> > display controller driver, modules don't seem to care overly much about
> > the powergate's state except that it needs to be turned on before they
> > touch some of the registers.
> > 
> > From a bit of experimentation it also seems like the sequence encoded
> > within tegra_powergate_sequence_power_up() isn't at all necessary. I
> > couldn't find an authoritative reference for that either, so I'm tempted
> > to conclude that it was simply cargo-culted from the dark-ages.
> > 
> > So I'm thinking that if we ever move to use power domains for this, we
> > may be able to just drop any extra handling (well, we'd need to keep it
> > for backwards-compatibility... *sigh*) and let drivers handle the clock
> > and reset resources.
> > 
> > On the other hand, given that we already need to keep the existing code
> > for backwards-compatibility, I'm not sure there's a real advantage in
> > turning them into power domains, since we'd be adding extra code without
> > an clear gains (especially since it seems like we'd need even more code
> > to properly handle suspend/resume in drivers that need powergates).
> > 
> 
> Unless we fix the framework to require context save/restore for suspend.
> There is a good reason to do that. context save/restore requires energy
> as well, so it's not a given that turning off domains in system suspend
> will save power.

I'm not sure I follow. "require context save/restore for suspend" is
what many drivers currently don't support, hence we can't use the
generic PM domains. Perhaps what you're saying is that the PM domain
core code should be enhanced so that domains can be marked so that they
stay on during a suspend/resume cycle.

Such functionality could be part of a specifier in a DT binding. It's
technically not a description of the hardware, but it encodes a device's
requirements regarding suspend/resume latency.

Thierry
Peter De Schrijver July 9, 2014, 8:33 a.m. UTC | #15
On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > 
> > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > someday which means we would need to control the domain and therefore would
> > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > driver doesn't know anything about the ISP hw otherwise.
> > > 
> > > Can't we make powergates reference counted so that they don't get
> > > disabled as long as there are any users? Looking for example at the
> > 
> > We could, but then why not switch to the powerdomain code and make powering
> > off a domain a NOP until we sorted out the context save/restore or fixed
> > the framework to allow for suspend without turning off the domains?
> 
> Well, one of the reasons why I'm not sure it's worth the effort at this
> point is that we can't get rid of the tegra_powergate_*() API anyway
> because of backwards compatibility. So we're going to add code (without
> getting rid of old code) merely to support some generic framework. That
> doesn't sound very useful to me.
> 

We can also convert the existing users to genpd. Today there are only 2 users
(gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
an impossible task.

> > > display controller driver, modules don't seem to care overly much about
> > > the powergate's state except that it needs to be turned on before they
> > > touch some of the registers.
> > > 
> > > From a bit of experimentation it also seems like the sequence encoded
> > > within tegra_powergate_sequence_power_up() isn't at all necessary. I
> > > couldn't find an authoritative reference for that either, so I'm tempted
> > > to conclude that it was simply cargo-culted from the dark-ages.
> > > 
> > > So I'm thinking that if we ever move to use power domains for this, we
> > > may be able to just drop any extra handling (well, we'd need to keep it
> > > for backwards-compatibility... *sigh*) and let drivers handle the clock
> > > and reset resources.
> > > 
> > > On the other hand, given that we already need to keep the existing code
> > > for backwards-compatibility, I'm not sure there's a real advantage in
> > > turning them into power domains, since we'd be adding extra code without
> > > an clear gains (especially since it seems like we'd need even more code
> > > to properly handle suspend/resume in drivers that need powergates).
> > > 
> > 
> > Unless we fix the framework to require context save/restore for suspend.
> > There is a good reason to do that. context save/restore requires energy
> > as well, so it's not a given that turning off domains in system suspend
> > will save power.
> 
> I'm not sure I follow. "require context save/restore for suspend" is
> what many drivers currently don't support, hence we can't use the
> generic PM domains. Perhaps what you're saying is that the PM domain
> core code should be enhanced so that domains can be marked so that they
> stay on during a suspend/resume cycle.
> 

Exactly.

Cheers,

Peter.
Thierry Reding July 9, 2014, 10:25 a.m. UTC | #16
On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > 
> > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > someday which means we would need to control the domain and therefore would
> > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > 
> > > > Can't we make powergates reference counted so that they don't get
> > > > disabled as long as there are any users? Looking for example at the
> > > 
> > > We could, but then why not switch to the powerdomain code and make powering
> > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > the framework to allow for suspend without turning off the domains?
> > 
> > Well, one of the reasons why I'm not sure it's worth the effort at this
> > point is that we can't get rid of the tegra_powergate_*() API anyway
> > because of backwards compatibility. So we're going to add code (without
> > getting rid of old code) merely to support some generic framework. That
> > doesn't sound very useful to me.
> > 
> 
> We can also convert the existing users to genpd. Today there are only 2 users
> (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> an impossible task.

We can certainly do that. What I'm trying to say is that since people
may be running newer versions of the kernel with a DTB that doesn't have
the necessary properties to hook up power domains, we have to keep calls
to tegra_powergate_*() functions as-is, lest we break those setups.

That then means that each such driver needs a way to figure out whether
power domains are hooked up (which will be rather clumsy to do if it's
all supposed to work transparently) and fallback to the legacy API
otherwise.

So essentially we'll be adding code without the possibility to remove
any of the old code.

Thierry
Lucas Stach July 9, 2014, 10:31 a.m. UTC | #17
Am Mittwoch, den 09.07.2014, 12:25 +0200 schrieb Thierry Reding:
> On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > > * PGP Signed by an unknown key
> > > 
> > > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > > 
> > > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > > someday which means we would need to control the domain and therefore would
> > > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > > 
> > > > > Can't we make powergates reference counted so that they don't get
> > > > > disabled as long as there are any users? Looking for example at the
> > > > 
> > > > We could, but then why not switch to the powerdomain code and make powering
> > > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > > the framework to allow for suspend without turning off the domains?
> > > 
> > > Well, one of the reasons why I'm not sure it's worth the effort at this
> > > point is that we can't get rid of the tegra_powergate_*() API anyway
> > > because of backwards compatibility. So we're going to add code (without
> > > getting rid of old code) merely to support some generic framework. That
> > > doesn't sound very useful to me.
> > > 
> > 
> > We can also convert the existing users to genpd. Today there are only 2 users
> > (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> > an impossible task.
> 
> We can certainly do that. What I'm trying to say is that since people
> may be running newer versions of the kernel with a DTB that doesn't have
> the necessary properties to hook up power domains, we have to keep calls
> to tegra_powergate_*() functions as-is, lest we break those setups.
> 
> That then means that each such driver needs a way to figure out whether
> power domains are hooked up (which will be rather clumsy to do if it's
> all supposed to work transparently) and fallback to the legacy API
> otherwise.
> 
> So essentially we'll be adding code without the possibility to remove
> any of the old code.
> 
You could also take the easy way out and enable all power domains by
default. So drivers won't be able to disable the power domain on old
DTBs where they aren't hooked up, but the will continue to function
without calling into the old Tegra specific power domain code.

Regards,
Lucas
Peter De Schrijver July 9, 2014, 11:08 a.m. UTC | #18
On Wed, Jul 09, 2014 at 12:25:52PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > > 
> > > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > > 
> > > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > > someday which means we would need to control the domain and therefore would
> > > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > > 
> > > > > Can't we make powergates reference counted so that they don't get
> > > > > disabled as long as there are any users? Looking for example at the
> > > > 
> > > > We could, but then why not switch to the powerdomain code and make powering
> > > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > > the framework to allow for suspend without turning off the domains?
> > > 
> > > Well, one of the reasons why I'm not sure it's worth the effort at this
> > > point is that we can't get rid of the tegra_powergate_*() API anyway
> > > because of backwards compatibility. So we're going to add code (without
> > > getting rid of old code) merely to support some generic framework. That
> > > doesn't sound very useful to me.
> > > 
> > 
> > We can also convert the existing users to genpd. Today there are only 2 users
> > (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> > an impossible task.
> 
> We can certainly do that. What I'm trying to say is that since people
> may be running newer versions of the kernel with a DTB that doesn't have
> the necessary properties to hook up power domains, we have to keep calls
> to tegra_powergate_*() functions as-is, lest we break those setups.
> 

For those 2 domains we can find the necessary clocks and resets by parsing
the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
even needed as we can always register some extra clkdev's to get them. There
is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
but that's not too bad I think.

Cheers,

Peter.
Peter De Schrijver July 9, 2014, 11:20 a.m. UTC | #19
On Wed, Jul 09, 2014 at 12:31:47PM +0200, Lucas Stach wrote:
> > 
> You could also take the easy way out and enable all power domains by
> default. So drivers won't be able to disable the power domain on old
> DTBs where they aren't hooked up, but the will continue to function
> without calling into the old Tegra specific power domain code.
> 

If the bootloader turned some domains off, the kernel would have to turn them
back on again. However that requires knowledge about the clocks and resets of
the peripherals in the domain.

Cheers,

Peter.
Thierry Reding July 9, 2014, 12:04 p.m. UTC | #20
On Wed, Jul 09, 2014 at 02:08:16PM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 12:25:52PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Wed, Jul 09, 2014 at 11:33:11AM +0300, Peter De Schrijver wrote:
> > > On Wed, Jul 09, 2014 at 08:31:32AM +0200, Thierry Reding wrote:
> > > > > Old Signed by an unknown key
> > > > 
> > > > On Tue, Jul 08, 2014 at 05:11:35PM +0300, Peter De Schrijver wrote:
> > > > > > > 
> > > > > > > Yes, but the problem is that you also need clocks and reset of other modules
> > > > > > > in the same domain to safely control the domain's status. Eg: the ISPs, VI and
> > > > > > > CSI share a domain. VI and CSI are useable without ISP and the ISP lacks
> > > > > > > public documentation. So it's not unlikely a VI and CSI driver will upstreamed
> > > > > > > someday which means we would need to control the domain and therefore would
> > > > > > > need to tell that driver about the ISPs clocks and resets even though the
> > > > > > > driver doesn't know anything about the ISP hw otherwise.
> > > > > > 
> > > > > > Can't we make powergates reference counted so that they don't get
> > > > > > disabled as long as there are any users? Looking for example at the
> > > > > 
> > > > > We could, but then why not switch to the powerdomain code and make powering
> > > > > off a domain a NOP until we sorted out the context save/restore or fixed
> > > > > the framework to allow for suspend without turning off the domains?
> > > > 
> > > > Well, one of the reasons why I'm not sure it's worth the effort at this
> > > > point is that we can't get rid of the tegra_powergate_*() API anyway
> > > > because of backwards compatibility. So we're going to add code (without
> > > > getting rid of old code) merely to support some generic framework. That
> > > > doesn't sound very useful to me.
> > > > 
> > > 
> > > We can also convert the existing users to genpd. Today there are only 2 users
> > > (gpu/drm/tegra/gr3d.c and pci/host/pci-tegra.c), so that doesn't seem to be
> > > an impossible task.
> > 
> > We can certainly do that. What I'm trying to say is that since people
> > may be running newer versions of the kernel with a DTB that doesn't have
> > the necessary properties to hook up power domains, we have to keep calls
> > to tegra_powergate_*() functions as-is, lest we break those setups.
> > 
> 
> For those 2 domains we can find the necessary clocks and resets by parsing
> the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> even needed as we can always register some extra clkdev's to get them. There
> is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> but that's not too bad I think.

Even if we could really do this, at this point I don't see an advantage.
All that it would be doing is move to some subsystem that doesn't quite
match what we need just for the sake of moving to that subsystem. Having
a Tegra-specific API doesn't sound so bad anymore.

Thierry
Peter De Schrijver July 9, 2014, 12:43 p.m. UTC | #21
On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > For those 2 domains we can find the necessary clocks and resets by parsing
> > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > even needed as we can always register some extra clkdev's to get them. There
> > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > but that's not too bad I think.
> 
> Even if we could really do this, at this point I don't see an advantage.
> All that it would be doing is move to some subsystem that doesn't quite
> match what we need just for the sake of moving to that subsystem. Having
> a Tegra-specific API doesn't sound so bad anymore.
> 

The advantage would be that we can use LP0/SC7 as a cpuidle state. Also system
resume from LP0 can be faster as we potentially don't have to resume all
domains at once.

Cheers,

Peter.
Thierry Reding July 9, 2014, 12:56 p.m. UTC | #22
On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > even needed as we can always register some extra clkdev's to get them. There
> > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > but that's not too bad I think.
> > 
> > Even if we could really do this, at this point I don't see an advantage.
> > All that it would be doing is move to some subsystem that doesn't quite
> > match what we need just for the sake of moving to that subsystem. Having
> > a Tegra-specific API doesn't sound so bad anymore.
> > 
> 
> The advantage would be that we can use LP0/SC7 as a cpuidle state.

How is that going to work? And why does it need powergates to be
implemented as power domains? If we switch off power gates, then we need
to restore context in the drivers anyway, therefore I assume .suspend()
and .resume() would need to be called, in which case powergate handling
can surely be done at that stage, can't it?

> Also system
> resume from LP0 can be faster as we potentially don't have to resume all
> domains at once.

I don't understand what that's got to do with anything. If we call into
the PMC driver explicitly via tegra_powergate_*() functions from driver
code, then we have full control over suspend/resume in the drivers, and
therefore don't need to resume all at once either.

Thierry
Peter De Schrijver July 9, 2014, 1:20 p.m. UTC | #23
On Wed, Jul 09, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > > even needed as we can always register some extra clkdev's to get them. There
> > > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > > but that's not too bad I think.
> > > 
> > > Even if we could really do this, at this point I don't see an advantage.
> > > All that it would be doing is move to some subsystem that doesn't quite
> > > match what we need just for the sake of moving to that subsystem. Having
> > > a Tegra-specific API doesn't sound so bad anymore.
> > > 
> > 
> > The advantage would be that we can use LP0/SC7 as a cpuidle state.
> 
> How is that going to work? And why does it need powergates to be

pm_runtime_get() and pm_runtime_put() hook into genpd. So genpd knows
when all devices in a domain are idle. It can then decide to turn off
the domain (based on the decision of a per domain governor). If all
domains are off (except for the non-powergateable domain), a special cpuidle
state can be enabled by genpd which will initiate a transition to LP0 without
actually doing a full system suspend.

> implemented as power domains? If we switch off power gates, then we need
> to restore context in the drivers anyway, therefore I assume .suspend()
> and .resume() would need to be called, in which case powergate handling
> can surely be done at that stage, can't it?
> 

.suspend() and .resume() are not used for this. genpd uses other per device
callbacks to save and restore the state which are invoked when the domain
is turned off and on (.save_state and .restore_state). The major difference
with .suspend() and .resume() is that .suspend() has to perform 3 tasks:
prevent any new requests to the driver, finalize or cancel all outstanding
requests and save the hw context. .save_state will only be called when the
device is idle (based on the refcount controlled by pm_runtime_get() and
pm_runtime_put()) which means it only has to handle saving the hw context.

> > Also system
> > resume from LP0 can be faster as we potentially don't have to resume all
> > domains at once.
> 
> I don't understand what that's got to do with anything. If we call into
> the PMC driver explicitly via tegra_powergate_*() functions from driver
> code, then we have full control over suspend/resume in the drivers, and
> therefore don't need to resume all at once either.

But then we would be duplicating all the bookkeeping required for this? What's
the point of that?

Cheers,

Peter.
Thierry Reding July 9, 2014, 2:42 p.m. UTC | #24
On Wed, Jul 09, 2014 at 04:20:10PM +0300, Peter De Schrijver wrote:
> On Wed, Jul 09, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> > > On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > > > even needed as we can always register some extra clkdev's to get them. There
> > > > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > > > but that's not too bad I think.
> > > > 
> > > > Even if we could really do this, at this point I don't see an advantage.
> > > > All that it would be doing is move to some subsystem that doesn't quite
> > > > match what we need just for the sake of moving to that subsystem. Having
> > > > a Tegra-specific API doesn't sound so bad anymore.
> > > > 
> > > 
> > > The advantage would be that we can use LP0/SC7 as a cpuidle state.
> > 
> > How is that going to work? And why does it need powergates to be
> 
> pm_runtime_get() and pm_runtime_put() hook into genpd. So genpd knows
> when all devices in a domain are idle. It can then decide to turn off
> the domain (based on the decision of a per domain governor). If all
> domains are off (except for the non-powergateable domain), a special cpuidle
> state can be enabled by genpd which will initiate a transition to LP0 without
> actually doing a full system suspend.

Okay, I see.

> > implemented as power domains? If we switch off power gates, then we need
> > to restore context in the drivers anyway, therefore I assume .suspend()
> > and .resume() would need to be called, in which case powergate handling
> > can surely be done at that stage, can't it?
> > 
> 
> .suspend() and .resume() are not used for this. genpd uses other per device
> callbacks to save and restore the state which are invoked when the domain
> is turned off and on (.save_state and .restore_state). The major difference
> with .suspend() and .resume() is that .suspend() has to perform 3 tasks:
> prevent any new requests to the driver, finalize or cancel all outstanding
> requests and save the hw context. .save_state will only be called when the
> device is idle (based on the refcount controlled by pm_runtime_get() and
> pm_runtime_put()) which means it only has to handle saving the hw context.

With the above, would it be possible to make turning off the domain
conditional on whether or not all devices in the domain implement
.save_state() and .restore_state()? That would allow us to convert to
power domains and then stage in context save/restore in drivers (or even
leave it out if there's not enough to be gained from turning the
partition off).

> > > Also system
> > > resume from LP0 can be faster as we potentially don't have to resume all
> > > domains at once.
> > 
> > I don't understand what that's got to do with anything. If we call into
> > the PMC driver explicitly via tegra_powergate_*() functions from driver
> > code, then we have full control over suspend/resume in the drivers, and
> > therefore don't need to resume all at once either.
> 
> But then we would be duplicating all the bookkeeping required for this? What's
> the point of that?

We're doing fine without any bookkeeping currently. I understand that
this may change eventually, but I'm hesitant to start any conversion
like this before we don't have a better understanding of how it should
work (and actual use-cases which we can test). Also we've seen in the
past that coding things up before we have enough use-cases we're bound
to fail at coming up with a proper binding and then we have to keep
carrying loads of code for compatibility.

So if you're willing to give this a shot, I'm not at all opposed to it
generally. But we need to make sure that both the binding is reasonably
future-proof and that we can actually test things like reference-counted
power domains.

Now in the meantime there are a bunch of other drivers that will need to
use the powergate API. DC is one of them. We haven't needed this before
because we assumed the partitions would be on by default. That's not
always the case apparently (ChromeOS does some funky things here). Both
the SATA and XUSB drivers that have been posted use them as well and the
nouveau driver that Alex has been working on uses at least parts of it.
I don't think it's fair to keep them from being merged while we're
trying to make the transition to power domains, but we should keep an
eye on what's happening there so it doesn't conflict with any of the
work we're planning for power domains.

Thierry
Peter De Schrijver July 9, 2014, 4:09 p.m. UTC | #25
On Wed, Jul 09, 2014 at 04:42:18PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jul 09, 2014 at 04:20:10PM +0300, Peter De Schrijver wrote:
> > On Wed, Jul 09, 2014 at 02:56:14PM +0200, Thierry Reding wrote:
> > > > Old Signed by an unknown key
> > > 
> > > On Wed, Jul 09, 2014 at 03:43:44PM +0300, Peter De Schrijver wrote:
> > > > On Wed, Jul 09, 2014 at 02:04:02PM +0200, Thierry Reding wrote:
> > > > > > For those 2 domains we can find the necessary clocks and resets by parsing
> > > > > > the relevant existing DT nodes for PCIe and gr3d. For clocks, this isn't
> > > > > > even needed as we can always register some extra clkdev's to get them. There
> > > > > > is no equivalent for resets so we have to parse the gr3d and pcie DT nodes,
> > > > > > but that's not too bad I think.
> > > > > 
> > > > > Even if we could really do this, at this point I don't see an advantage.
> > > > > All that it would be doing is move to some subsystem that doesn't quite
> > > > > match what we need just for the sake of moving to that subsystem. Having
> > > > > a Tegra-specific API doesn't sound so bad anymore.
> > > > > 
> > > > 
> > > > The advantage would be that we can use LP0/SC7 as a cpuidle state.
> > > 
> > > How is that going to work? And why does it need powergates to be
> > 
> > pm_runtime_get() and pm_runtime_put() hook into genpd. So genpd knows
> > when all devices in a domain are idle. It can then decide to turn off
> > the domain (based on the decision of a per domain governor). If all
> > domains are off (except for the non-powergateable domain), a special cpuidle
> > state can be enabled by genpd which will initiate a transition to LP0 without
> > actually doing a full system suspend.
> 
> Okay, I see.
> 
> > > implemented as power domains? If we switch off power gates, then we need
> > > to restore context in the drivers anyway, therefore I assume .suspend()
> > > and .resume() would need to be called, in which case powergate handling
> > > can surely be done at that stage, can't it?
> > > 
> > 
> > .suspend() and .resume() are not used for this. genpd uses other per device
> > callbacks to save and restore the state which are invoked when the domain
> > is turned off and on (.save_state and .restore_state). The major difference
> > with .suspend() and .resume() is that .suspend() has to perform 3 tasks:
> > prevent any new requests to the driver, finalize or cancel all outstanding
> > requests and save the hw context. .save_state will only be called when the
> > device is idle (based on the refcount controlled by pm_runtime_get() and
> > pm_runtime_put()) which means it only has to handle saving the hw context.
> 
> With the above, would it be possible to make turning off the domain
> conditional on whether or not all devices in the domain implement
> .save_state() and .restore_state()? That would allow us to convert to
> power domains and then stage in context save/restore in drivers (or even
> leave it out if there's not enough to be gained from turning the
> partition off).
> 

Maybe. I would have to check that.

> > > > Also system
> > > > resume from LP0 can be faster as we potentially don't have to resume all
> > > > domains at once.
> > > 
> > > I don't understand what that's got to do with anything. If we call into
> > > the PMC driver explicitly via tegra_powergate_*() functions from driver
> > > code, then we have full control over suspend/resume in the drivers, and
> > > therefore don't need to resume all at once either.
> > 
> > But then we would be duplicating all the bookkeeping required for this? What's
> > the point of that?
> 
> We're doing fine without any bookkeeping currently. I understand that
> this may change eventually, but I'm hesitant to start any conversion
> like this before we don't have a better understanding of how it should
> work (and actual use-cases which we can test). Also we've seen in the
> past that coding things up before we have enough use-cases we're bound
> to fail at coming up with a proper binding and then we have to keep
> carrying loads of code for compatibility.
> 
> So if you're willing to give this a shot, I'm not at all opposed to it
> generally. But we need to make sure that both the binding is reasonably
> future-proof and that we can actually test things like reference-counted
> power domains.
> 
> Now in the meantime there are a bunch of other drivers that will need to
> use the powergate API. DC is one of them. We haven't needed this before
> because we assumed the partitions would be on by default. That's not
> always the case apparently (ChromeOS does some funky things here). Both
> the SATA and XUSB drivers that have been posted use them as well and the
> nouveau driver that Alex has been working on uses at least parts of it.
> I don't think it's fair to keep them from being merged while we're
> trying to make the transition to power domains, but we should keep an
> eye on what's happening there so it doesn't conflict with any of the
> work we're planning for power domains.

The problem with this is that moving to the genpd APIs will become much more
difficult I'm afraid. I think we should maybe just make the pmc driver turn
on all the domains which were turned off by the bootloader. That way the
drivers don't need to handle the powerdomains at all for the time being.

Cheers,

Peter.
diff mbox

Patch

diff --git a/arch/arm/mach-tegra/powergate.c b/arch/arm/mach-tegra/powergate.c
index 50a9af4..254c42e 100644
--- a/arch/arm/mach-tegra/powergate.c
+++ b/arch/arm/mach-tegra/powergate.c
@@ -125,6 +125,7 @@  int tegra_powergate_power_on(int id)
 
 	return tegra_powergate_set(id, true);
 }
+EXPORT_SYMBOL(tegra_powergate_power_on);
 
 int tegra_powergate_power_off(int id)
 {