[RFC] powerpc: Merge 32/64 cacheflush code

[David Gibson]

On Wed, Dec 21, 2005 at 11:16:01AM -0600, Milton Miller wrote:
> 
> On Dec 19, 2005, at 7:06 PM, David Gibson wrote:
> 
> >On Mon, Dec 19, 2005 at 08:52:47AM -0600, Milton Miller wrote:
> >>On Mon Dec 19 16:44:10 EST 2005, David Gibson wrote:
> >>
> >>>+extern void wback_dcache_range(unsigned long start, unsigned long
> >>>stop);
> >>>+extern void wback_inval_dcache_range(unsigned long start, unsigned
> >>>long stop);
> >>
> >>I think that while we are here we should change the arguments to be
> >>pointers (void *).   The assembly doesn't care, and almost all of the
> >>users are casting from pointer to usigned long at the call site, with
> >>dart being the exception.  The instruction cache flush should also
> >>change.
> >
> >True.  And while we're at that, the dcache flushing functions are
> >almost invariable called as *_dcache_range(start, start+length), so
> >how about changing them to take start and length instead of start and
> >end.  However, flush_icache_range() is called from generic code, so I
> >don't want to change it's interface.
> 
> The information from the above paragraph should be in the change log.

Duly added.

> And if we are not going to change icache, then I would propose we not 
> use range on the dcache side, it is confusing.   How about rename to th 
> start, length ones to _region ?

Hrm.. yes.  Paulus, do you have a preferred approach?

> >Revised patch below:
> >
> >powerpc: Merge 32/64 cacheflush code
> >
> >This patch merges the cache flushing code for 32 and 64 bit powerpc
> >machines.  This means the ppc64_caches mechanism for determining
> >correct cache sizes at runtime is ported to 32-bit, and is thus
> >renamed as 'powerpc_caches'.  The merged cache flushing functions go
> >in new file arch/powerpc/kernel/cache.S.
> >
> >Previously, the ppc32 version of flush_dcache_range() did a writeback
> >and invalidate of the given cache lines (dcbf) whereas the ppc64
> >version did just a writeback (dcbst).  In general, there's no
> >consistent meaning of "flush" as one or the other, so this patch also
> >renames the dcache flushing functions less ambiguously.  The new names
> >are:
> >
> >	wback_dcache_range() - previously flush_dcache_range() on
> >ppc64 and clean_dcache_range() on ppc32
> >
> >	wback_inval_dcache_range() - previously
> >flush_inval_dcache_range() on ppc64 and flush_dcache_range on ppc32
> >
> >	invalidate_dcache_range() - didn't previously exist on ppc64,
> >unchanged on ppc32
> >
> >Finally we also cleanup the initialization of the powerpc_caches
> >structure from the old ppc64 specific version.  We remove a pointless
> >loop, and remove a dependence on _machine.
> 
> ...
> 
> >+/*
> >+ * Flush a particular page from the data cache to RAM.
> >+ * Note: this is necessary because the instruction cache does *not*
> >+ * snoop from the data cache.
> >+ *
> >+ *	void __flush_dcache_icache(void *page)
> 
> When I see *page i think struct page *page .... even though this is 
> void, how about page_address ?

Good point, though not my addition (that's copied verbatim from the
existing misc_64.S.  Changed to page_va, as in the prototype in
cacheflush.h

> >+/*
> >+ * Like above, but only do the D-cache.
> >+ *
> >+ * wback_dcache_range(void *start, unsigned long len)
> >+ *
> >+ *    writeback all bytes from start to stop-1 inclusive
> >+ */
> >+_GLOBAL(wback_dcache_range)
> >+ 	LOAD_REG_ADDR(r10, powerpc_caches)
> >+	lwz	r7,DCACHEL1LINESIZE(r10)	/* Get dcache line size */
> >+	addi	r5,r7,-1
> >+	andc	r6,r3,r5		/* round low to line bdy */
> >+	and	r8,r3,r5		/* get cacheline offset of start */
> 
> Does the above term ever make a difference since we round up?

Yes.  Consider wback_dcache_range((void*)0x7f, 2).

> >+	add	r8,r8,r4		/* add length */
> >+	add	r8,r8,r5		/* ensure we get enough */
> >+	lwz	r9,DCACHEL1LOGLINESIZE(r10)	/* Get dcache line shift */
> 
> Moving this load up might help some processors

I'd rather make such a change as a separate patch, this code path is
identical to the existing ppc64 code.

> >+	srw.	r8,r8,r9		/* compute line count */
> 
> So its really unsigned int len (or did you want a PPC_ macro here?)

Hmm.. good point.  The old versions also had this, though there it was
more subtle - a difference of pointers being truncated to 32bits.  I
can see no reason not to use an srd. on 64-bit here, PPC_SRL macro
duly added.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson