Re: [PATCH 9/9] powerpc/pm: support deep sleep feature on T1040

[Kevin Hao]

On Fri, Mar 14, 2014 at 05:26:27PM -0500, Scott Wood wrote:
> On Thu, 2014-03-13 at 15:46 +0800, Kevin Hao wrote:
> > On Wed, Mar 12, 2014 at 12:43:05PM -0500, Scott Wood wrote:
> > > > Shouldn't we use "readback, sync" here? The following is quoted form t4240RM:
> > > >   To guarantee that the results of any sequence of writes to configuration
> > > >   registers are in effect, the final configuration register write should be
> > > >   immediately followed by a read of the same register, and that should be
> > > >   followed by a SYNC instruction. Then accesses can safely be made to memory
> > > >   regions affected by the configuration register write.
> > > 
> > > I agree that the sync before the readback is probably not necessary,
> > > since transactions to the same address should already be ordered.
> > > 
> > > A sync after the readback helps if you're trying to order the readback
> > > with subsequent memory accesses, though in that case wouldn't a sync
> > > alone (no readback) be adequate?
> > 
> > No, we don't just want to order the subsequent memory access here.
> > The 'write, readback, sync' is the required sequence if we want to make
> > sure that the writing to CCSR register does really take effect.
> > 
> > >  Though maybe not always -- see the
> > > comment near the end of fsl_elbc_write_buf() in
> > > drivers/mtd/nand_fsl_elbc.c.  I guess the readback does more than just
> > > make sure the device has seen the write, ensuring that the device has
> > > finished the transaction to the point of acting on another one.
> > 
> > Agree.
> > 
> > > 
> > > The data dependency plus isync sequence, which is done by the normal I/O
> > > accessors used from C code, orders the readback versus all future
> > > instructions (not just I/O).  The delay loop is not I/O.
> > 
> > According to the PowerISA, the sequence 'load, date dependency, isync' only
> > order the load accesses. 
> 
> The point is to order the delay loop after the load, not to order
> storage versus storage.

I think the point is to make sure that the writing of the CCSR_DDR_SDRAM_CFG_2
does really take effect before we begin to delay loop. The sequence "write,
readback, sync" will guarantee this according to the manual. If we just want to
order the delay loop after the load, the following sequence should be enough:
	store to CCSR_DDR_SDRAM_CFG_2
	load from CCSR_DDR_SDRAM_CFG_2
	isync or sync
	delay loop

Why do we need the 'date dependency' here? According to the e6500 manual, the
instructions can execute out of order, but complete in order. So I am really
wondering why we need to order the load and the following delay loop if there
is no intention to order the storage access? 

> 
> This is a sequence we're already using on all of our I/O loads
> (excluding accesses like in this patch that don't use the standard
> accessors).  I'm confident that it works even if it's not
> architecturally guaranteed.

This sequence is used to order the load and the followed storage access.
And this is guaranteed by the architecture. But I don't think it is suitable
for a case like this. The following is quoted from PowerISA:
  Because stores cannot be performed âout-of-orderâ
  (see Book III), if a Store instruction depends on the
  value returned by a preceding Load instruction
  (because the value returned by the Load is used to
  compute either the effective address specified by the
  Store or the value to be stored), the corresponding stor-
  age accesses are performed in program order. The
  same applies if whether the Store instruction is exe-
  cuted depends on a conditional Branch instruction that
  in turn depends on the value returned by a preceding
  Load instruction.
  
  Because an isync instruction prevents the execution of
  instructions following the isync until instructions pre-
  ceding the isync have completed, if an isync follows a
  conditional Branch instruction that depends on the
  value returned by a preceding Load instruction, the
  load on which the Branch depends is performed before
  any loads caused by instructions following the isync.

I think the above description would guarantee that the load will be performed
before any storage access (both load and store) following the isync in the
following scenario:
	lwz	r4, 0(r3)
	twi	0, r4, 0
	isync

>  I'm not sure that there exists a clear
> architectural way of synchronizing non-storage instructions relative to
> storage instructions.

Isn't what the execution synchronization instructions such as sync, isync, mtmsr
do?

> 
> Given that isync is documented as preventing any execution of
> instructions after the isync until all previous instructions complete,
> it doesn't seem to make sense for the architecture to explicitly talk
> about loads (as opposed to any other instruction) following a load,
> dependent conditional branch, isync sequence.

Sorry, I didn't get what you mean.

> 
> > So if we want to order all the storage access as well
> > as execution synchronization, we should choose sync here.
> 
> Do we need execution synchronization or context synchronization?

There is no context-altering instruction here, so I think an execution
synchronizing instruction should be enough here.

> 
> The t4240 RM section that talks about a readback and a sync is in the
> context of subsequent memory operations ("Then accesses can safely be
> made to memory regions affected..."), not arbitrary instructions.

I assume that this sequence also guarantee that the writing does take effect.

>  There
> are also a couple other places in the RM where isync is recommended
> instead (when setting LAWs or CCSRBAR), even though those also only
> involve memory accesses.

No idea why isync is used here.

> 
> In any case, this is not performance critical and thus it's better to
> oversynchronize than undersynchronize.

On the contrary I think that sync is oversynchronize instead of
undersynchronize. It not only provide the execution synchronizing but also
order all the storage accesses. That is why I prefer the sync. :-)

Thanks,
Kevin

Attachment: pgp2Om2X_hbry.pgp
Description: PGP signature