[PATCH] GEODE: decouple sleep/resume from powerdown/powerup (take 2)

[Bernardo Innocenti]

Andres Salomon wrote:

>> diff --git a/drivers/video/geode/lxfb_ops.c b/drivers/video/geode/lxfb_ops.c
>> index b2ecb4d..b380238 100644
>> --- a/drivers/video/geode/lxfb_ops.c
>> +++ b/drivers/video/geode/lxfb_ops.c
>> @@ -829,33 +829,42 @@ static void lx_restore_regs(struct fb_info *info, struct geoderegs *regs)
> [...]
>> -int lx_shutdown(struct fb_info *info)
>> +int lx_power(struct fb_info *info, int state)
>>  {
>>  	struct lxfb_par *par = info->par;
>>  
>> -	if (lx_power_on == 0)
>> -		return 0;
>> -
>> -	writel(DC_UNLOCK_CODE, par->dc_regs + DC_UNLOCK);
>> -	lx_save_regs(info, &saved_regs);
>> -	lx_graphics_disable(info);
>> +	if (state < 0 || state > 3)
>> +		return -EINVAL;
>>  
>> -	lx_power_on = 0;
>> -	return 0;
>> -}
>> +	/* Going into power save */
>> +	if (state >= 2 && lx_power_state < 2)
>> +		lx_graphics_disable(info);
>>  
>> -int lx_powerup(struct fb_info *info)
>> -{
>> -	struct lxfb_par *par = info->par;
>> +	/* Powering down */
>> +	if (state >= 3 && lx_power_state < 3) {
>> +		writel(DC_UNLOCK_CODE, par->dc_regs + DC_UNLOCK);
>> +		lx_save_regs(info, &saved_regs);
>> +	}
>>  
>> -	if (lx_power_on == 1)
>> -		return 0;
>> +	/* Restoring from power save */
>> +	if (state < 3 && lx_power_state >= 3) {
>> +		lx_restore_regs(info, &saved_regs);
>> +		writel(0, par->dc_regs + DC_UNLOCK);
>> +	}
>>  
>> -	lx_restore_regs(info, &saved_regs);
>> -	writel(0, par->dc_regs + DC_UNLOCK);
>> +	/* Powering up */
>> +	if (state < 2 && lx_power_state >= 2)
>> +		lx_graphics_enable(info);
>>  
>> -	lx_power_on = 1;
>> +	lx_power_state = state;
>>  	return 0;
> 
> 
> Jordan claims that we don't want to be calling lx_{save,restore}_regs
> for state 2...  I'll let him explain.

We aren't doing that already.

We save registers only on the transition from 2 to 3 and we restore them
when we go back from 3 to 2.

On the GX, we *save* them always and restore them only on powerup.
Saving the registers is needed because, later on, we peek a few values
from that structure for powering up the video unit.


> Jordan and I discussed using fb_blank() rather than adding an additional API.

Was it on IRC or email?  Could you please forward it to me?


> Basically, my feeling is that fb_blank is already doing (or should be doing)
> what we want to be doing (disabling misc hardware bits that are not needed
> when the video processor is no longer pulling images from the GPU).  So, we
> might as well just be calling fb_blank from the dcon's sleep/freeze functions.
> Jordan prefers the fb_power* stuff.  We can argue about it later, with
> upstream.

Using the fb_blank() interface seems like a very good idea to me.  The different
kinds of blanking can be mapped to DCON freeze, video off, and complete poweroff.

This rework will take some time.  Could we push it after Trial3?  For now, I see
this patch as a quick bugfix only.  And the change brings the fb_power() API
somewhat *closer* to what would be needed for switching to fb_


> Also, the fb_power() API really needs a stricter definition of power levels.

I looked into pci_power_t and it looks close to what we need:

 #define PCI_D0          ((pci_power_t __force) 0)
 #define PCI_D1          ((pci_power_t __force) 1)
 #define PCI_D2          ((pci_power_t __force) 2)
 #define PCI_D3hot       ((pci_power_t __force) 3)
 #define PCI_D3cold      ((pci_power_t __force) 4)
 #define PCI_UNKNOWN     ((pci_power_t __force) 5)
 #define PCI_POWER_ERROR ((pci_power_t __force) -1)

But this change will have to be undone later if we move to fb_blank().
To save time, couldn't we just elide it?

-- 
 \___/
 |___|  Bernardo Innocenti - http://www.codewiz.org/
  \___\ One Laptop Per Child - http://www.laptop.org/