Re: [RFC][PATCH 0/2] Export platform_pci_set_power_state() and make radeonfb use it

[Alex Deucher <alexdeucher@gmail.com>]

On 3/23/09, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 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...

I don't see any errata for this, but it's hard to find stuff as chips get older.

Alex