lkml.org 
[lkml]   [2009]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH 0/2] Export platform_pci_set_power_state() and make radeonfb use it
On Tue, 24 Mar 2009 11:57:19 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Mon, 2009-03-23 at 15:23 -0700, Jesse Barnes wrote:
> > The thing I didn't like was that it made the radeon driver use an
> > internal interface; I'd really prefer a proper return value from
> > pci_set_power_state, which in turn means auditing all its current
> > callers. But that doesn't seem worth it unless we see other drivers
> > needing something similar...
> >
> > And if we did go with something like your first patch, I'd still
> > rather see the timeout done in the driver, rather than having the
> > attempts & delay included in the function...
>
> So what ? The driver would call pci_set_power_state() until it stops
> failing ?

Yeah, that's what I had in mind.

> I'm not too fan of that, because it will change the access pattern
> to the chip:
>
> - write PM to 2
> - short delay
> - read PM, see 0, return error
> - driver does big delay
> - write PM to 2
> - short delay
> - read PM ....
>
> vs. the current sequence which is
>
> - write PM to 2
> - long delay
> - read PM, be happy
>
> Which -seems- to be pretty much what happens in practice, though on
> that chip, I don't know for sure about others.
>
> I'm worried of the possible side effects of the first sequence that
> you propose since it would do 2 things potentially confusing to the
> HW:
>
> - read PM after a short delay... it -should- be harmless but you know
> HW as well as I do ...
>
> - write PM to 2 a second time after the long delay. Again, it
> -should- be harmless since the chip at this stage should already be
> in D2 state but god knows how the HW will react.
>
> I'm especially worried about the later in fact. Maybe we can minimize
> it by having pci_set_power_state() dbl check the content of the PM
> reg before writing to it...

Honestly I'm not too happy about any of the approaches, but yeah I see
your point. The main thing is to prevent any config space access for
a specified time after the first D-state transition, which I think we
do correctly in the core. Beyond that, we just have to make sure the
core state is updated correctly; Rafael's first patch does that
correctly I think.

Actually now that I think of it, maybe Alex can get us details on the
errata here. If we just need a longer wait between a D-state transition
and the next config space access for this chip (or set of chips), we
could add that as a proper quirk to the core...

Alex, any thoughts about the bug & patch in
http://bugzilla.kernel.org/show_bug.cgi?id=12846 ? Looks like old
radeon chips need a workaround when transitioning from D0 -> D2...

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center


\
 
 \ /
  Last update: 2009-03-24 02:17    [W:0.090 / U:0.712 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site