VME driver patch for PowerPC

VME driver patch for PowerPC

[Oliver Korpilla]

Hello!

Could you check, whether the patch to your driver (version 7433-3.2 of your
Linux support) still compiles on an Intel platform, and works as intended?

Problem is, this is not complete:

Direct referencing of the obtained memory pointer will lead to "bad" PCI
accesses, 8 long words in a row, writing to the window will trigger a read
(again 8  long words) and a failed write afterwards.

But using standard readl() and writel()  on the ioremap_nocache'd addresses will
work just fine (just except that it swaps the byte-order, from PCI little endian
to host big endian), with correct and proper accesses.

So dereferencing the pointer has bad results, using memcpy() as well, but kernel
macros work fine.

Either this is still a cache problem, and if so, I may or may not find and solve
it, or I have to use appropriate macros in read() and write() calls, and
implement those - this would work on both platforms.

Patch follows below (it's not really much - minimally different from what I've
already sent you, but it's the minimal change that will produce PCI accesses,
and correct ones, if used with readl()/writel()).

With kind regards,
Oliver Korpilla

diff -urN vme_universe/module/vme_master.c vme_universe-new/module/vme_master.c
--- vme_universe/module/vme_master.c	2004-06-07 17:16:13.000000000 +0200
+++ vme_universe-new/module/vme_master.c	2004-06-07 17:13:43.000000000 +0200
@@ -163,6 +163,65 @@
  MODULE_PARM(master_window6, "3-4i");
  MODULE_PARM(master_window7, "3-4i");

+extern struct pci_dev *universe_pci_dev;
+
+/* Try getting a resource (range of PCI addresses) from the PCI bus we're on */
+static int allocate_pci_resource(unsigned long size, unsigned long align,
+				 struct resource *new_resource) {
+  /* Determine the bus the Tundra is on */
+  struct pci_bus *bus = universe_pci_dev->bus;
+  int i;
+
+  for (i=0; i<4; i++) {
+    int retval;
+    struct resource *r = bus->resource[i];
+
+    /* Check if that resource exists */
+    if (!r)
+      continue;
+
+    /* If the resource is not I/O memory (e.g. I/O ports) */
+    if (! (r->flags & IORESOURCE_MEM))
+      continue;
+
+#ifdef DEBUG
+    /* Print out name of resource for debugging */
+    if (r->name)
+      printk(KERN_INFO "Checking bus resource with name \"%s\".\n", r->name);
+    printk(KERN_INFO "resource.start: %08lX, resource.end: %08lX.\n",
+	   r->start, r->end);
+#endif
+
+    /* Try to allocate a new sub-resource from this
+       given the proper size and alignment*/
+    retval = allocate_resource(r, new_resource, size,
+			       pci_lo_bound, pci_hi_bound,
+			       align, NULL, NULL);
+
+    /* If this allocation fails, try with next resource
+       (and give debug message) */
+    if (retval < 0) {
+
+#ifdef DEBUG
+      if (r->name)
+	printk(KERN_INFO
+	       "Failed allocating from bus resource with name \"%s\".\n",
+	       r->name);
+      else
+	printk(KERN_INFO
+	       "Failed allocating from bus resource with number %d.\n", i);
+#endif
+
+      continue;
+    }
+    /* If this allocation succeeds, return what allocate_resource() returned */
+    else
+      return retval;
+  }
+
+  /* return busy if no resource could be successfully allocated */
+  return -EBUSY;
+}

  /*============================================================================
   * Hook for display proc page info
@@ -428,9 +487,8 @@
  			return rval;
  		}
  	} else {
-		rval = allocate_resource(&iomem_resource, &window->resource,
-					 size, pci_lo_bound, pci_hi_bound,
-					 resolution, NULL, NULL);
+		rval = allocate_pci_resource(size, resolution, &window->resource);
+
  		if (rval) {
  			window->resource.start = 0;
  			window->resource.end = 0;
@@ -622,9 +680,8 @@

  	/* Allocate a 64MB window with 64kb resolution
  	 */
-	rval = allocate_resource(&iomem_resource, &slsi_window.resource,
-				 0x4000000, pci_lo_bound, pci_hi_bound,
-				 0x10000, NULL, NULL);
+	rval = allocate_pci_resource(0x4000000, 0x10000, &slsi_window.resource);
+
  	if (rval) {
  		printk(KERN_WARNING "VME: Unable to allocate memory for SLSI "
  		       "window\n");

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

RE: VME driver patch for PowerPC

[Heater, Daniel (GE Infrastructure)]

> Could you check, whether the patch to your driver (version
> 7433-3.2 of your
> Linux support) still compiles on an Intel platform, and works
> as intended?

Yep. I only did a quick test, but it appears to work fine on x86.
I've merged your patch up with the code base for the next release.

I think I have an idea for the read/write question, but I need to
look at it a little closer. I'll get back to you in a bit.

Thanks,
Daniel L. Heater
Software Development, Embedded Systems
GE Fanuc Automation Americas, Inc.
VMIC, Inc.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: VME driver patch for PowerPC

[Oliver Korpilla]

Heater, Daniel (GE Infrastructure) wrote:

>>Could you check, whether the patch to your driver (version
>>7433-3.2 of your
>>Linux support) still compiles on an Intel platform, and works
>>as intended?
>>
>>
>
>Yep. I only did a quick test, but it appears to work fine on x86.
>I've merged your patch up with the code base for the next release.
>
>
>
Great!

I don't know about x86 bus organization, but maybe it limits the address
range for available addresses too strongly (on a single bus, where the
Universe is on). Comparing how much window space you can map with the
old version and the new one on x86 could prove useful.

>I think I have an idea for the read/write question, but I need to
>look at it a little closer. I'll get back to you in a bit.
>
>
With a little bit of "luck" maybe there's a way around it, and I'm
currently looking into it:

readb(), readw() and readl() work fine (and their writeb/w/l
counterparts do as well), but pointer derefencing does not. Every
pointer dereference does trigger an 8 long word read from the bus first.
I asked what could trigger a long read from a single dereference of
memory? Cache. Not surprisingly cache line length of my current
development board (MVME2100) is 8 long words for Cache Burst
Transactions. So I have the wanted single-beat transactions when using
readx()/writex(), as documented when using cache-inhibited, guarded or
write-through memory or if the cache is disabled, but am getting cache
bursts on pointers.

I will take a closer look at the properties of the pages I'm using in
kernel space and in user space, and the page tables, and maybe that
fixes it.

BTW, everywhere where you use pci_alloc_consistent (dma, slaves), it
ports perfectly fine.

With kind regards,
Oliver Korpilla

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: VME driver patch for PowerPC

[Oliver Korpilla]

Hello!

I tried dereferencing the pointer in kernel space like this:

unsigned long int *virtaddr = 0;

// [...]

// After the Universe register were written in
// __create_master_window()
virtaddr = ioremap_nocache(window->phys_base, window->size);
printk(KERN_INFO "Dereferenced pointer 0x%08X.\n", *virtaddr);


Guess what that produced: A single-beat transaction producing the date within
the expected time constraints without a cache burst or any other "bad stuff".

So kernel space pages are fine, correctly set to cache-inhibited and guarded (no
reordering of accesses).

The culprit could be the vme_mmap_phys() function, because it introduces another
mapping of pages, and with mmap you cannot control caching behaviour.

With kind regards,
Oliver Korpilla

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: VME driver patch for PowerPC

[Oliver Korpilla]

Hello!

Adding the patch below activated correct behaviour for vme_peek/poke, with
correct data width (tested VME_D8, VME_D16 and VME_D32), and without cache bursts.

It simply sets the cache-inhibited and guarded bits before remapping the pages
(these flags are PowerPC-specific, and the pci_mmap_page_range() function is
sadly no exported kernel symbol).

Would you again be so kind to run some "still works"-test on an Intel board?

Looks like a hack, but I guess it simply gets the job done... No need for a
larger rewrite here.

May take some time till I can again lay hands on a MVME5500 (PPC 7455 - 1Ghz) to
verify on another PPC board, though.

With kind regards,
Oliver Korpilla

Index: module/vme_main.c
===================================================================
--- module/vme_main.c   (revision 5)
+++ module/vme_main.c   (revision 6)
@@ -191,14 +191,18 @@
   */
  int vme_mmap(struct file *file_ptr, struct vm_area_struct *vma)
  {
-
         DPRINTF("Attempting to map %#lx bytes of memory at "
                 "physical address %#lx\n", vma->vm_end - vma->vm_start,
                 vma->vm_pgoff << PAGE_SHIFT);

+#ifdef CONFIG_PPC32
+       vma->vm_page_prot.pgprot |= _PAGE_NO_CACHE | _PAGE_GUARDED;
+       DPRINTF("PowerPC protection flags set.\n");
+#endif
+
         /* Don't swap these pages out
          */
-       vma->vm_flags |= VM_RESERVED;
+       vma->vm_flags |= VM_LOCKED | VM_IO | VM_SHM;

  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,3) || defined RH9BRAINDAMAGE
         return remap_page_range(vma, vma->vm_start, vma->vm_pgoff << PAGE_SHIFT,

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

RE: VME driver patch for PowerPC

[Heater, Daniel (GE Infrastructure)]

> +#ifdef CONFIG_PPC32
> +       vma->vm_page_prot.pgprot |= _PAGE_NO_CACHE | _PAGE_GUARDED;
> +       DPRINTF("PowerPC protection flags set.\n");
> +#endif

Cool. I was just about to suggest _PAGE_NO_CACHE | _PAGE_GUARDED.

>          /* Don't swap these pages out
>           */
> -       vma->vm_flags |= VM_RESERVED;
> +       vma->vm_flags |= VM_LOCKED | VM_IO | VM_SHM;

I'm trying to understand this change. VM_IO looks like it needs to
be there to prevent deadlocks on core dumps.
http://www.uwsg.iu.edu/hypermail/linux/kernel/0202.0/1309.html

and if I'm interpreting some older mailing list postings correctly,
VM_RESERVED is a replacement for VM_LOCKED | VM_SHM but VM_RESERVED
may yield some performance advantages. Thus, in later kernels you
only see VM_RESERVED and not VM_LOCKED | VM_SHM.

Maybe since this is an out of tree driver, it should have
> +       vma->vm_flags |= VM_LOCKED | VM_IO | VM_SHM | VM_RESERVED;

to handle older kernels and still get the advantages of VM_RESERVED
on newer kernels.

What do you think? Am I interpreting this correctly?

Daniel.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: VME driver patch for PowerPC

[Oliver Korpilla]

Heater, Daniel (GE Infrastructure) wrote:
>>         /* Don't swap these pages out
>>          */
>>-       vma->vm_flags |= VM_RESERVED;
>>+       vma->vm_flags |= VM_LOCKED | VM_IO | VM_SHM;
>
>
> I'm trying to understand this change. VM_IO looks like it needs to
> be there to prevent deadlocks on core dumps.
> http://www.uwsg.iu.edu/hypermail/linux/kernel/0202.0/1309.html
>
> and if I'm interpreting some older mailing list postings correctly,
> VM_RESERVED is a replacement for VM_LOCKED | VM_SHM but VM_RESERVED
> may yield some performance advantages. Thus, in later kernels you
> only see VM_RESERVED and not VM_LOCKED | VM_SHM.
>
> Maybe since this is an out of tree driver, it should have
>
>>+       vma->vm_flags |= VM_LOCKED | VM_IO | VM_SHM | VM_RESERVED;
>
>
> to handle older kernels and still get the advantages of VM_RESERVED
> on newer kernels.
>
> What do you think? Am I interpreting this correctly?
>

To be frank, I "modelled" this after the flag configuration in
pci_mmap_page_range() in the PowerPC tree of kernel 2.4.21 (where I got the page
protection changes, too).

If VM_RESERVED is somewhat of an alias, it should prove okay. Looking into my
"documentation" yielded no quick results for flag interpretation.

I could simply test, if both works.

With kind regards,
Oliver Korpilla

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

RE: VME driver patch for PowerPC

[Heater, Daniel (GE Infrastructure)]

> >>Could you check, whether the patch to your driver (version
> >>7433-3.2 of your
> >>Linux support) still compiles on an Intel platform, and works
> >>as intended?
> >>
> >Yep. I only did a quick test, but it appears to work fine on x86.
> >I've merged your patch up with the code base for the next release.
> >
> Great!
>
> I don't know about x86 bus organization, but maybe it limits
> the address
> range for available addresses too strongly (on a single bus,
> where the
> Universe is on). Comparing how much window space you can map with the
> old version and the new one on x86 could prove useful.

Your dead on right. I cannot allocate as much space. In fact,
on a VMIVME-7750 I'm only able to allocate 768KB now vs. the
~2MB I could access before.

> Adding the patch below activated correct behaviour for vme_peek/poke,
> with correct data width (tested VME_D8, VME_D16 and VME_D32), and
> without cache bursts.
>
> It simply sets the cache-inhibited and guarded bits before
> remapping the pages (these flags are PowerPC-specific, and the
> pci_mmap_page_range() function is sadly no exported kernel symbol).
>
> Would you again be so kind to run some "still works"-test on an
> Intel board?

That part still works.

Thanks,
Daniel L. Heater
Software Development, Embedded Systems
GE Fanuc Automation Americas, Inc.
VMIC, Inc.


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/