Yet another ccio fix idea?
Quoted 10 lines
Quoted 6 lines
asm
volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[7]))Flushing
pdir_ptrmay or may not work depending on the cache line width (pdr_ptr[7] is 28 bytes away, so if the line size is 16 it will fail; although I think almost every machine with a ccio has a 32 byte or higher cache line width).Actually,
pdr_ptris a char *, so this is 7 bytes away. However, I think you are correct in saying that pdr_ptr[7] needs to be flushed.
Quoted 16 lines
The same issue would appear to apply to the other flush in the file.
The cache line alignment of pdir's isn't immediately apparent to me. If the pdir was defined as a two word array, it might span two lines.
Dave
Signature · 6 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602) -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
mmm follwing stuff shouldn't so be enough:
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +3 -2
@@ -624,7 +624,8 @@ ** the real mode coherence index generation of U2, the PDIR entry ** must be flushed to memory to retain coherence." */ - asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr)); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[1])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[0])); asm volatile("sync"); }
@@ -695,7 +696,7 @@ ** Hopefully someone figures out how to patch (NOP) the ** FDC/SYNC out at boot time. */ - asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr[7])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[7])); iovp += IOVP_SIZE; byte_cnt -= IOVP_SIZE;
=== <> ===
Specialy for the first hunk where pdir_ptr is a u64 *:
ccio_io_pdir_entry(u64 *pdir_ptr, space_t sid, unsigned long vba,
unsigned long hints)
into which with temporary pa var (defined as an unsigned long)
register unsigned long pa;
*pdir_ptr is changed like
((u32 *)pdir_ptr)[1] = (u32) pa;
[snip]
((u32 *)pdir_ptr)[0] = (u32) pa;What would be the best way to works?
Tx,
r.
PS: in this doc feb96a6.pdf (easy to find) it was said:
"[snip]
Entries in the PA 7200 and PA 8000 caches are stored in lines of 32 bytes.
[snip]
Because one-word writes occur in the I/O system,
for registers, semaphores, or short DMA writes it was necessary that the I/O adapter implement a one-line-deep cache to
buffer cache lines, so that these one-word writes could be executed by performing a coherent read private transaction on
the Runway bus, obtaining the most recent copy of the cache line, modifying it locally in cache, and finally writing the
modified line back to main memory. For the I/O adapter to support a cache on the Runway bus, it has to have the ability to
compare processor-generated virtual address transactions with the address contained in its cache to ensure that the
processors always receive the most up-to-date data.
[s
[snip]"
2 thinks:
- it seems to confirm that cache line is well 32bytes wide ;-)
- for 'one word writes...' how should it be implemented? It look like the sba_iommu driver:
"READ_REG(ioc-ioc_ha_IOC_PCOM); /* flush purges */
but without more detailed docs on U2/UTurn ccio, I don't know how to implement matter, sorry.
---
Scarlet One, ADSL 6 Mbps + Telephone, from EUR 29,95...
http://www.scarlet.be/
James Bottomley
maintainerdrivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 line 629
Quoted 4 lines · rubisher
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +2
-1- asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr)); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[1])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[0])); asm volatile("sync");
Actually, no, I'm afraid this is wrong.
The code is a complete dogs breakfast, since pdir_ptr is defined as u64
* in ccio_pdir_entry() and char * in ccio_mark_invalid.
What Dave was pointing out is that this structure is 8 bytes long and may span two cache lines on the word boundary. Assuming that's the case, this needs to flush the first and second word, not the first and second u64 entry as your code would (the second flush is actually flushing the next pdir entry).
However, since it's contained in a u64 pointer, it must reasonably be on an 8 byte boundary (or be triggering an unaligned poiner error) and therefore shouldn't span multiple cache lines anyway.
James
Quoted 3 lines
However, since it's contained in a u64 pointer, it must reasonably be on an 8 byte boundary (or be triggering an unaligned poiner error) and therefore shouldn't span multiple cache lines anyway.
It's not a hard rule that u64 objects be aligned on an 8 byte boundary as the PA 1.x architecture doesn't have 64-bit integer loads and stores. A u64 object has to be loaded with two 32-bit loads and lives in a pair of 32-bit registers. As a result, an unaligned u64 object will only trigger an unaligned pointer error if it is not aligned on a 4 byte boundary.
However in GCC, we force u64 alignment (except in packed structs) to a 8 byte boundary. This allows casting pointers between long longs and doubles. It also simplifies the GCC backend as otherwise special treatment would be needed for long longs. There's been some discussion of this for x86 recently.
In this situation, the alignment of pdir's depends on how they were originally declared. If declared as long long, GCC will give them 8 byte alignment, so the two words should always be in the same cache line. On the otherhand, if the pdir is declared as a pair of 32-bit words, the pdir might span two cache lines.
Dave
Signature · 2 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
Quoted 5 lines
In this situation, the alignment of pdir's depends on how they were originally declared. If declared as long long, GCC will give them 8 byte alignment, so the two words should always be in the same cache line. On the otherhand, if the pdir is declared as a pair of 32-bit words, the pdir might span two cache lines.
It looks to me like pdirs should be 8 byte aligned and thus always should lie in a cache line.
The following insn appears to put hints in the wrong place on a 64-bit kernel since `pa' is then a 64-bit type:
asm volatile("depw %1,31,12,%0" : "+r" (pa) : "r" (hints));The depw will operate on the left word. Looks like it should be
asm volatile("depw %1,31,12,%R0" : "+r" (pa) : "r" (hints));when __LP64__ is defined. Think the same is true for the following depw as well.
Dave
Signature · 2 lines
J. David Anglin [email protected] National Research Council of Canada (613) 990-0752 (FAX: 952-6602)
Quoted 10 lines · James Bottomley
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 line 629
Quoted 4 lines · rubisher
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +2
-1- asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr)); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[1])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[0])); asm volatile("sync");Actually, no, I'm afraid this is wrong.
The code is a complete dogs breakfast, since
pdir_ptris defined as u64
* inccio_pdir_entry()and char * inccio_mark_invalid.
*ugh* sorry about that.
And pdir_ptr cast to "u32 *" in both uses where it's dereferenced.
Quoted 9 lines
What Dave was pointing out is that this structure is 8 bytes long and may span two cache lines on the word boundary. Assuming that's the case, this needs to flush the first and second word, not the first and second u64 entry as your code would (the second flush is actually flushing the next pdir entry).
However, since it's contained in a u64 pointer, it must reasonably be on an 8 byte boundary (or be triggering an unaligned poiner error) and therefore shouldn't span multiple cache lines anyway.
pdir_ptr will always point at a u64 * and will always be 8 byte aligned. The allocation is done in ccio_ioc_init():
ioc->pdir_base = (u64 *)__get_free_pages(GFP_KERNEL,
get_order(ioc->pdir_size));hth,
grant
Grant Grundler
NAKOn Wed, Mar 12, 2008 at 10:16:29AM +0100, rubisher wrote:
...
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 line 628
Quoted 12 lines
mmm follwing stuff shouldn't so be enough:
Index:b/drivers/parisc/ccio-dma.c
===================================================================drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +2
-1@@ -624,7 +624,8 @@ ** the real mode coherence index generation of U2, the PDIR entry ** must be flushed to memory to retain coherence." */ - asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr)); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[1])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[0]));
NACK
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 line 699
Quoted 9 lines
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +1
-1asm volatile("sync"); }@@ -695,7 +696,7 @@ ** Hopefully someone figures out how to patch (NOP) the ** FDC/SYNC out at boot time. */ - asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr[7])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[7]));
NACK - keep it simple with the original proposed patch.
Quoted 13 lines
PS: in this doc feb96a6.pdf (easy to find) it was said:
"[snip]
Entries in the PA 7200 and PA 8000 caches are stored in lines of 32 bytes.
[snip]Because one-word writes occur in the I/O system, for registers, semaphores, or short DMA writes it was necessary that the I/O adapter implement a one-line-deep cache to buffer cache lines, so that these one-word writes could be executed by performing a coherent read private transaction on the Runway bus, obtaining the most recent copy of the cache line, modifying it locally in cache, and finally writing the modified line back to main memory.
...
Quoted 4 lines
2 thinks:
- it seems to confirm that cache line is well 32bytes wide ;-)- for 'one word writes...' how should it be implemented?
"one word writes" refers to DMA writes, not CPU writes.
Quoted 4 lines
It look like the
sba_iommudriver:
"READ_REG(ioc-ioc_ha_IOC_PCOM); /* flush purges */
but without more detailed docs on U2/UTurn ccio, I don't know how to implement matter, sorry.
The IOMMU driver programs the IO Pdir with the results of "lci" instruction and this should be all the IOMMU needs to coherently read/modify/write the cacheline that the "one word" DMA write is targeting. In short, it's already implemented or I'd think we'd see much worse problems on ccio platforms.
hth,
grant
Quoted 6 lines
---
Scarlet One, ADSL 6 Mbps + Telephone, from EUR 29,95...
http://www.scarlet.be/
PATCH 1/1 ccio-dma.c [Was: Yet another ccio fix idea?]
Quoted 20 lines · Grant Grundler
On Wed, Mar 12, 2008 at 10:16:29AM +0100, rubisher wrote:
...drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 line 628
Quoted 12 lines
mmm follwing stuff shouldn't so be enough:
Index:b/drivers/parisc/ccio-dma.c
===================================================================drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +2
-1@@ -624,7 +624,8 @@ ** the real mode coherence index generation of U2, the PDIR entry ** must be flushed to memory to retain coherence." */ - asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr)); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[1])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[0]));NACK
Quoted 3 lines
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +0
-0asm volatile("sync"); }
Quoted 9 lines
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 line 699
Quoted 6 lines
drivers/parisc/ccio-dma.c 2008-03-12 08:50:54.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +1
-1@@ -695,7 +696,7 @@ ** Hopefully someone figures out how to patch (NOP) the ** FDC/SYNC out at boot time. */ - asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr[7])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[7]));NACK - keep it simple with the original proposed patch.
Ok here it is:
Signed-off-by: Joel Soete <soete dot joel at scarlet dot be>
--- drivers/parisc/ccio-dma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/parisc/ccio-dma.c 2008-03-15 19:19:52.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +1 -1
@@ -695,7 +695,7 @@ ** Hopefully someone figures out how to patch (NOP) the ** FDC/SYNC out at boot time. */ - asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr[7])); + asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr)); iovp += IOVP_SIZE; byte_cnt -= IOVP_SIZE;
=== <> ===
[snip]
Hth,
r.
Re: [Patch] ccio-dma.c [Was: Yet another ccio fix idea?]
On Sat, Mar 15, 2008 at 07:21:15PM +0000, rubisher wrote:
...
Quoted 5 lines
Quoted 3 lines
Review
Quoted 2 lines
(quoted diff) +1
-1- asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr[7])); + asm volatile("fdc %%r0(%0)" : : "r" (&pdir_ptr[7]));NACK - keep it simple with the original proposed patch.
Ok here it is:
Signed-off-by: Joel Soete <soete dot joel at scarlet dot be>
Kyle,
please apply :)
thanks,
grant
Quoted 17 lines
--- drivers/parisc/ccio-dma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
b/drivers/parisc/ccio-dma.c
===================================================================drivers/parisc/ccio-dma.c 2008-03-15 19:19:52.000000000 +0000 renamed from drivers/parisc/ccio-dma.c 2008-03-12 08:49:20.000000000 +0000 +1
-1@@ -695,7 +695,7 @@ ** Hopefully someone figures out how to patch (NOP) the ** FDC/SYNC out at boot time. */ - asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr[7])); + asm volatile("fdc %%r0(%0)" : : "r" (pdir_ptr)); iovp += IOVP_SIZE; byte_cnt -= IOVP_SIZE;
Grant Grundler wrote:
[snip]
Quoted 13 lines
Quoted 6 lines
PS: in this doc feb96a6.pdf (easy to find) it was said:
"[snip]
Entries in the PA 7200 and PA 8000 caches are stored in lines of 32 bytes.
[snip]Quoted 7 lines
Because one-word writes occur in the I/O system, for registers, semaphores, or short DMA writes it was necessary that the I/O adapter implement a one-line-deep cache to buffer cache lines, so that these one-word writes could be executed by performing a coherent read private transaction on the Runway bus, obtaining the most recent copy of the cache line, modifying it locally in cache, and finally writing the modified line back to main memory.
Quoted 9 lines
...
Quoted 4 lines
2 thinks:
- it seems to confirm that cache line is well 32bytes wide ;-)- for 'one word writes...' how should it be implemented?
"one word writes" refers to DMA writes, not CPU writes.
(tx it's not always easy to follow the master and the slave in this story)
Quoted 5 lines
Quoted 4 lines
It look like the
sba_iommudriver:
"READ_REG(ioc-ioc_ha_IOC_PCOM); /* flush purges */
but without more detailed docs on U2/UTurn ccio, I don't know how to implement matter, sorry.
Quoted 9 lines
The IOMMU driver programs the IO Pdir with the results of "lci" instruction and this should be all the IOMMU needs to coherently read/modify/write the cacheline that the "one word" DMA write is targeting. In short, it's already implemented or I'd think we'd see much worse problems on ccio platforms.
hth,
grant
Ok.
Just one thought because in the backport I did on ccio of your job on sba (which seems to works for me as well as previous stuff, may be a bit better for system low in ram like my c110), it was one of the matter related to io coherency which I didn't reach to backport?
Tx again,
r.