Proposed changes to io.h

Proposed changes to io.h

[John Whitney]

[-- Attachment #1: Type: text/plain, Size: 4078 bytes --]

I've made few changes to include/asm-ppc/io.h that might want to be
incorporated into the mainline tree.  These changes include:

1. Modifications to virt_to_bus, bus_to_virt, virt_to_phys, and
phys_to_virt.  With the use of fully virtual addresses for
cache-coherent allocations (consistent_alloc(), etc.), just subracting
KERNELBASE from the virtual address is no longer sufficient.  Because
of this, I have modified virt_to_phys and phys_to_virt to look like:

extern inline unsigned long virt_to_phys(volatile void * address)
{
     unsigned long phys_addr;
     unsigned long virt_addr = (unsigned long) address;

#ifndef CONFIG_APUS
     if ((virt_addr >= KERNELBASE) && (virt_addr < (unsigned long)
high_memory))
         phys_addr = (virt_addr - KERNELBASE);
     else
         phys_addr = iopa (virt_addr);
#else
     phys_addr = iopa (virt_addr);
#endif

     return phys_addr;
}

extern inline void * phys_to_virt(unsigned long address)
{
#ifndef CONFIG_APUS
     return (void *) (address + KERNELBASE);
#else
     return (void*) mm_ptov (address);
#endif
}

And I have modified virt_to_bus and bus_to_virt to use those inlined
functions:

extern inline unsigned long virt_to_bus(volatile void * address)
{
     if (address == (void *) 0)
         return 0;

     return virt_to_phys (address) + PCI_DRAM_OFFSET;
}

extern inline void * bus_to_virt(unsigned long address)
{
     if (address == 0)
         return 0;

     return phys_to_virt (address - PCI_DRAM_OFFSET);
}

This simplifies the "bus" routines, and makes sure that they use the
standard virtual/physical translations.


2. I'd like to add 64-bit __raw_readll and __raw_writell routines to
io.h, done using floating-point registers.  Currently, modules such as
MTD (when writing to 64-bit buses) perform two 32-bit, non-atomic
writes, which can cause problems.  Using a floating-point register to
guarantee a 64-bit write is ugly, but it works.  Code for these inlined
routines is as follows:

/*
  * For reading and writing 64-bit values, we need to use the floating
point
  * registers.  The code will enable MSR_FP in the MSR register, use
FPR1 to
  * read from and write to memory, and then restore everything to the
  * previous values.
  */
#define __raw_readll __raw_readll
static inline unsigned long long __raw_readll (int addr)
{
     unsigned long flags;
     unsigned long msr;
     unsigned long msr_save;
     unsigned long long result;
     unsigned long long fp_save;

     local_irq_save (flags);
     asm volatile ("sync\n"
                   "mfmsr    %0\n"
                   "ori      %1,%0,0x2000\n"
                   "mtmsr    %1\n"
                   "isync\n"
                   "stfdx    1,0,%2\n"
                   "lfdx     1,0,%4\n"
                   "stfdx    1,0,%3\n"
                   "sync\n"
                   "lfdx     1,0,%2\n"
                   "mtmsr    %0\n"
                   "sync\n"
                   "isync\n"
                   : "=&r" (msr_save), "=&r" (msr)
                   : "r" (&fp_save), "r" (&result), "r" (addr)
                   : "memory" );
     local_irq_restore (flags);

     return result;
}

#define __raw_writell __raw_writell
static inline void __raw_writell (unsigned long long value, int addr)
{
     unsigned long flags;
     unsigned long msr;
     unsigned long msr_save;
     unsigned long long fp_save;

     local_irq_save (flags);
     asm volatile ("sync\n"
                   "mfmsr    %0\n"
                   "ori      %1,%0,0x2000\n"
                   "mtmsr    %1\n"
                   "isync\n"
                   "stfdx    1,0,%2\n"
                   "lfdx     1,0,%3\n"
                   "stfdx    1,0,%4\n"
                   "sync\n"
                   "lfdx     1,0,%2\n"
                   "mtmsr    %0\n"
                   "sync\n"
                   "isync\n"
                   : "=&r" (msr_save), "=&r" (msr)
                   : "r" (&fp_save), "r" (&value), "r" (addr)
                   : "memory" );
     local_irq_restore (flags);
     return;
}


Questions, comments, or flames?

John Whitney

[-- Attachment #2: io.h.patch --]
[-- Type: application/octet-stream, Size: 4949 bytes --]

--- kernel-2.6.5/include/asm-ppc/io.h	2004-03-29 13:36:34.000000000 -0500
+++ kernel-sbs-2.6.5/include/asm-ppc/io.h	2004-03-30 15:54:18.000000000 -0500
@@ -70,6 +70,72 @@
 #define __raw_writel(v, addr)	(*(volatile unsigned int *)(addr) = (v))

 /*
+ * For reading and writing 64-bit values, we need to use the floating point
+ * registers.  The code will enable MSR_FP in the MSR register, use FPR1 to
+ * read from and write to memory, and then restore everything to the
+ * previous values.
+ */
+#define __raw_readll __raw_readll
+static inline unsigned long long __raw_readll (int addr)
+{
+	unsigned long flags;
+	unsigned long msr;
+	unsigned long msr_save;
+	unsigned long long result;
+	unsigned long long fp_save;
+
+	local_irq_save (flags);
+	asm volatile ("sync\n"
+	              "mfmsr	%0\n"
+	              "ori		%1,%0,0x2000\n"
+	              "mtmsr	%1\n"
+	              "isync\n"
+	              "stfdx	1,0,%2\n"
+	              "lfdx		1,0,%4\n"
+	              "stfdx	1,0,%3\n"
+	              "sync\n"
+	              "lfdx		1,0,%2\n"
+	              "mtmsr	%0\n"
+	              "sync\n"
+	              "isync\n"
+	              : "=&r" (msr_save), "=&r" (msr)
+	              : "r" (&fp_save), "r" (&result), "r" (addr)
+	              : "memory" );
+	local_irq_restore (flags);
+
+	return result;
+}
+
+#define __raw_writell __raw_writell
+static inline void __raw_writell (unsigned long long value, int addr)
+{
+	unsigned long flags;
+	unsigned long msr;
+	unsigned long msr_save;
+	unsigned long long fp_save;
+
+	local_irq_save (flags);
+	asm volatile ("sync\n"
+	              "mfmsr	%0\n"
+	              "ori		%1,%0,0x2000\n"
+	              "mtmsr	%1\n"
+	              "isync\n"
+	              "stfdx	1,0,%2\n"
+	              "lfdx		1,0,%3\n"
+	              "stfdx	1,0,%4\n"
+	              "sync\n"
+	              "lfdx		1,0,%2\n"
+	              "mtmsr	%0\n"
+	              "sync\n"
+	              "isync\n"
+	              : "=&r" (msr_save), "=&r" (msr)
+	              : "r" (&fp_save), "r" (&value), "r" (addr)
+	              : "memory" );
+	local_irq_restore (flags);
+	return;
+}
+
+/*
  * The insw/outsw/insl/outsl macros don't do byte-swapping.
  * They are only used in practice for transferring buffers which
  * are arrays of bytes, and byte-swapping is not appropriate in
@@ -205,54 +271,56 @@
 			     unsigned int size, int flags);

 /*
- * The PCI bus is inherently Little-Endian.  The PowerPC is being
- * run Big-Endian.  Thus all values which cross the [PCI] barrier
- * must be endian-adjusted.  Also, the local DRAM has a different
- * address from the PCI point of view, thus buffer addresses also
- * have to be modified [mapped] appropriately.
+ * Change virtual addresses to physical addresses and vv, for
+ * addresses in the area where the kernel has the RAM mapped.
  */
-extern inline unsigned long virt_to_bus(volatile void * address)
+extern inline unsigned long virt_to_phys(volatile void * address)
 {
+	unsigned long phys_addr;
+	unsigned long virt_addr = (unsigned long) address;
+
 #ifndef CONFIG_APUS
-        if (address == (void *)0)
-		return 0;
-        return (unsigned long)address - KERNELBASE + PCI_DRAM_OFFSET;
+	if ((virt_addr >= KERNELBASE) && (virt_addr < (unsigned long) high_memory))
+		phys_addr = (virt_addr - KERNELBASE);
+	else
+		phys_addr = iopa (virt_addr);
 #else
-	return iopa ((unsigned long) address);
+	phys_addr = iopa (virt_addr);
 #endif
+
+	return phys_addr;
 }

-extern inline void * bus_to_virt(unsigned long address)
+extern inline void * phys_to_virt(unsigned long address)
 {
 #ifndef CONFIG_APUS
-        if (address == 0)
-		return 0;
-        return (void *)(address - PCI_DRAM_OFFSET + KERNELBASE);
+	return (void *) (address + KERNELBASE);
 #else
 	return (void*) mm_ptov (address);
 #endif
 }

 /*
- * Change virtual addresses to physical addresses and vv, for
- * addresses in the area where the kernel has the RAM mapped.
+ * The PCI bus is inherently Little-Endian.  The PowerPC is being
+ * run Big-Endian.  Thus all values which cross the [PCI] barrier
+ * must be endian-adjusted.  Also, the local DRAM has a different
+ * address from the PCI point of view, thus buffer addresses also
+ * have to be modified [mapped] appropriately.
  */
-extern inline unsigned long virt_to_phys(volatile void * address)
+extern inline unsigned long virt_to_bus(volatile void * address)
 {
-#ifndef CONFIG_APUS
-	return (unsigned long) address - KERNELBASE;
-#else
-	return iopa ((unsigned long) address);
-#endif
+	if (address == (void *) 0)
+		return 0;
+
+	return virt_to_phys (address) + PCI_DRAM_OFFSET;
 }

-extern inline void * phys_to_virt(unsigned long address)
+extern inline void * bus_to_virt(unsigned long address)
 {
-#ifndef CONFIG_APUS
-	return (void *) (address + KERNELBASE);
-#else
-	return (void*) mm_ptov (address);
-#endif
+	if (address == 0)
+		return 0;
+
+	return phys_to_virt (address - PCI_DRAM_OFFSET);
 }

 /*

Re: Proposed changes to io.h

[Eugene Surovegin]

On Wed, Mar 31, 2004 at 10:44:25AM -0500, John Whitney wrote:
> 2. I'd like to add 64-bit __raw_readll and __raw_writell routines to
> io.h, done using floating-point registers.  Currently, modules such as
> MTD (when writing to 64-bit buses) perform two 32-bit, non-atomic
> writes, which can cause problems.  Using a floating-point register to
> guarantee a 64-bit write is ugly, but it works.  Code for these inlined
> routines is as follows:

[snip]

I wonder will it work on 4xx CPUs which don't have floating point unit?

Eugene.


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

Re: Proposed changes to io.h

[Dan Malek]

Eugene Surovegin wrote:

> I wonder will it work on 4xx CPUs which don't have floating point unit?

It won't work on anything, will it?  Floating point isn't enabled for
the kernel, unless I missed that update.


	-- Dan


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

Re: Proposed changes to io.h

[Matt Porter]

On Wed, Mar 31, 2004 at 11:58:27AM -0500, Dan Malek wrote:
>
> Eugene Surovegin wrote:
>
> > I wonder will it work on 4xx CPUs which don't have floating point unit?
>
> It won't work on anything, will it?  Floating point isn't enabled for
> the kernel, unless I missed that update.

It will. The proposed changes turn on FP, save the scratch FP reg
context, and the restore everything (all under lock). Like was
mentioned, it's ugly but works. I know a few people doing this
(usually in custom MTD 64-bit access functions).

-Matt

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

Re: Proposed changes to io.h

[John Whitney]

On Mar 31, 2004, at 11:58 AM, Dan Malek wrote:

>
> Eugene Surovegin wrote:
>
>> I wonder will it work on 4xx CPUs which don't have floating point
>> unit?
>
> It won't work on anything, will it?  Floating point isn't enabled for
> the kernel, unless I missed that update.
>

This code specifically handles this by enabling the FP bit in the MSR,
bypassing the standard floating point handling.  It saves the contents
of FP0, uses it, restores the contents, restores the MSR, and exits.

John


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

Re: Proposed changes to io.h

[Dan Malek]

John Whitney wrote:

> This code specifically handles this .....


OK, already, enough of the e-mails, especially the personal ones. :-P

I was stupid and didn't read the code or comments, just the e-mail
explanation.  I don't normally read code unless it is in a patch
that can be compared to something so I can see what has changed.

Thanks.


	-- Dan


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

Re: Proposed changes to io.h

[Matt Porter]

On Wed, Mar 31, 2004 at 08:44:23AM -0800, Eugene Surovegin wrote:
>
> On Wed, Mar 31, 2004 at 10:44:25AM -0500, John Whitney wrote:
> > 2. I'd like to add 64-bit __raw_readll and __raw_writell routines to
> > io.h, done using floating-point registers.  Currently, modules such as
> > MTD (when writing to 64-bit buses) perform two 32-bit, non-atomic
> > writes, which can cause problems.  Using a floating-point register to
> > guarantee a 64-bit write is ugly, but it works.  Code for these inlined
> > routines is as follows:
>
> [snip]
>
> I wonder will it work on 4xx CPUs which don't have floating point unit?

#ifndef CONFIG_PPC_CLASSIC_FPU?

-Matt

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

Re: Proposed changes to io.h

[John Whitney]

I wonder will it work on 4xx CPUs which don't have floating point unit?
>
> #ifndef CONFIG_PPC_CLASSIC_FPU?
>

I don't see this in arch/ppc/Kconfig (or anywhere else) in the 2.6.5
kernel which I am using (or in arch/ppc/config.in, for 2.4.25).  I can
certainly create a patch to Kconfig that does this, or I could check
for PPC_FEATURE_HAS_FPU in cur_cpu_spec[0]->cpu_user_features  in
__raw_readq/writeq():

unsigned long long __raw_readq (int addr)
{
   if (cur_cpu_spec[0]->cpu_user_features & PPC_FEATURE_HAS_FPU) {
     ...
   } else {
     return *((volatile unsigned long long *) addr);
}

or somesuch.  This would work as long as the compiler doesn't have a
snitfit with FP instructions, even if the processor can't use them...

Which would the maintainers prefer?


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

Re: Proposed changes to io.h

[Matt Porter]

On Wed, Mar 31, 2004 at 02:57:14PM -0500, John Whitney wrote:
> I don't see this in arch/ppc/Kconfig (or anywhere else) in the 2.6.5
> kernel which I am using (or in arch/ppc/config.in, for 2.4.25).  I can
> certainly create a patch to Kconfig that does this, or I could check
> for PPC_FEATURE_HAS_FPU in cur_cpu_spec[0]->cpu_user_features  in
> __raw_readq/writeq():
>
> unsigned long long __raw_readq (int addr)
> {
>    if (cur_cpu_spec[0]->cpu_user_features & PPC_FEATURE_HAS_FPU) {
>      ...
>    } else {
>      return *((volatile unsigned long long *) addr);
> }
>
> or somesuch.  This would work as long as the compiler doesn't have a
> snitfit with FP instructions, even if the processor can't use them...
>
> Which would the maintainers prefer?

Here's my preference, hopefully Paul reads this. I personally don't
see why we need to use a runtime flag when we pretty much have the
info available at build time anyway.  If e500 wanted a version they
can use the SPE option to add some code. It might be better to warn
at build time for !fpu in addition to BUGging at runtime.

-Matt

===== arch/ppc/Kconfig 1.56 vs edited =====
--- 1.56/arch/ppc/Kconfig	Thu Mar 18 23:04:54 2004
+++ edited/arch/ppc/Kconfig	Wed Mar 31 14:06:56 2004
@@ -79,6 +79,11 @@
 	depends on 44x
 	default y

+config PPC_FPU
+	bool
+	depends on 6xx || POWER3 || POWER4
+	default y
+
 config ALTIVEC
 	bool "AltiVec Support"
 	depends on 6xx || POWER4
===== include/asm-ppc/io.h 1.19 vs edited =====
--- 1.19/include/asm-ppc/io.h	Fri Mar 12 14:17:57 2004
+++ edited/include/asm-ppc/io.h	Wed Mar 31 14:57:41 2004
@@ -69,6 +69,77 @@
 #define __raw_writew(v, addr)	(*(volatile unsigned short *)(addr) = (v))
 #define __raw_writel(v, addr)	(*(volatile unsigned int *)(addr) = (v))

+#ifdef CONFIG_PPC_FPU
+/*
+ * For reading and writing 64-bit values, we need to use the floating
+ * point registers.  The code will enable MSR_FP in the MSR register,
+ * use FPR1 to read from and write to memory, and then restore
+ * everything to the previous values.
+ */
+static inline unsigned long long ___raw_readq(int addr)
+{
+	unsigned long flags;
+	unsigned long msr;
+	unsigned long msr_save;
+	unsigned long long result;
+	unsigned long long fp_save;
+
+	local_irq_save (flags);
+	asm volatile ("sync\n"
+			"mfmsr    %0\n"
+			"ori      %1,%0,0x2000\n"
+			"mtmsr    %1\n"
+			"isync\n"
+			"stfdx    1,0,%2\n"
+			"lfdx     1,0,%4\n"
+			"stfdx    1,0,%3\n"
+			"sync\n"
+			"lfdx     1,0,%2\n"
+			"mtmsr    %0\n"
+			"sync\n"
+			"isync\n"
+			: "=&r" (msr_save), "=&r" (msr)
+			: "r" (&fp_save), "r" (&result), "r" (addr)
+			: "memory" );
+	local_irq_restore (flags);
+
+	return result;
+}
+
+static inline void ___raw_writeq(unsigned long long value, int addr)
+{
+	unsigned long flags;
+	unsigned long msr;
+	unsigned long msr_save;
+	unsigned long long fp_save;
+
+	local_irq_save (flags);
+	asm volatile ("sync\n"
+			"mfmsr    %0\n"
+			"ori      %1,%0,0x2000\n"
+			"mtmsr    %1\n"
+			"isync\n"
+			"stfdx    1,0,%2\n"
+			"lfdx     1,0,%3\n"
+			"stfdx    1,0,%4\n"
+			"sync\n"
+			"lfdx     1,0,%2\n"
+			"mtmsr    %0\n"
+			"sync\n"
+			"isync\n"
+			: "=&r" (msr_save), "=&r" (msr)
+			: "r" (&fp_save), "r" (&value), "r" (addr)
+			: "memory" );
+	local_irq_restore (flags);
+	return;
+}
+#define __raw_readq		___raw_readq
+#define __raw_writeq		___raw_writeq
+#else /* !CONFIG_PPC_FPU */
+#define __raw_readq(addr)		({ BUG(); })
+#define __raw_writeq(addr)		({ BUG(); })
+#endif /* CONFIG_PPC_FPU */
+
 /*
  * The insw/outsw/insl/outsl macros don't do byte-swapping.
  * They are only used in practice for transferring buffers which

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

Re: Proposed changes to io.h

[John Whitney]

Looks good for me.  The help for the 6xx choice indicated the 8xx
series (no FPU) is included in it, but I see it is actually in a
separate choice.  I'll post a Kconfig patch to clear that up on a
separate thread.

John

On Mar 31, 2004, at 5:07 PM, Matt Porter wrote:

> On Wed, Mar 31, 2004 at 02:57:14PM -0500, John Whitney wrote:
>> I don't see this in arch/ppc/Kconfig (or anywhere else) in the 2.6.5
>> kernel which I am using (or in arch/ppc/config.in, for 2.4.25).  I can
>> certainly create a patch to Kconfig that does this, or I could check
>> for PPC_FEATURE_HAS_FPU in cur_cpu_spec[0]->cpu_user_features  in
>> __raw_readq/writeq():
>>
>> unsigned long long __raw_readq (int addr)
>> {
>>    if (cur_cpu_spec[0]->cpu_user_features & PPC_FEATURE_HAS_FPU) {
>>      ...
>>    } else {
>>      return *((volatile unsigned long long *) addr);
>> }
>>
>> or somesuch.  This would work as long as the compiler doesn't have a
>> snitfit with FP instructions, even if the processor can't use them...
>>
>> Which would the maintainers prefer?
>
> Here's my preference, hopefully Paul reads this. I personally don't
> see why we need to use a runtime flag when we pretty much have the
> info available at build time anyway.  If e500 wanted a version they
> can use the SPE option to add some code. It might be better to warn
> at build time for !fpu in addition to BUGging at runtime.
>
> -Matt
>
> ===== arch/ppc/Kconfig 1.56 vs edited =====
> --- 1.56/arch/ppc/Kconfig	Thu Mar 18 23:04:54 2004
> +++ edited/arch/ppc/Kconfig	Wed Mar 31 14:06:56 2004
> @@ -79,6 +79,11 @@
>  	depends on 44x
>  	default y
>
> +config PPC_FPU
> +	bool
> +	depends on 6xx || POWER3 || POWER4
> +	default y
> +
>  config ALTIVEC
>  	bool "AltiVec Support"
>  	depends on 6xx || POWER4
> ===== include/asm-ppc/io.h 1.19 vs edited =====
> --- 1.19/include/asm-ppc/io.h	Fri Mar 12 14:17:57 2004
> +++ edited/include/asm-ppc/io.h	Wed Mar 31 14:57:41 2004
> @@ -69,6 +69,77 @@
>  #define __raw_writew(v, addr)	(*(volatile unsigned short *)(addr) =
> (v))
>  #define __raw_writel(v, addr)	(*(volatile unsigned int *)(addr) = (v))
>
> +#ifdef CONFIG_PPC_FPU
> +/*
> + * For reading and writing 64-bit values, we need to use the floating
> + * point registers.  The code will enable MSR_FP in the MSR register,
> + * use FPR1 to read from and write to memory, and then restore
> + * everything to the previous values.
> + */
> +static inline unsigned long long ___raw_readq(int addr)
> +{
> +	unsigned long flags;
> +	unsigned long msr;
> +	unsigned long msr_save;
> +	unsigned long long result;
> +	unsigned long long fp_save;
> +
> +	local_irq_save (flags);
> +	asm volatile ("sync\n"
> +			"mfmsr    %0\n"
> +			"ori      %1,%0,0x2000\n"
> +			"mtmsr    %1\n"
> +			"isync\n"
> +			"stfdx    1,0,%2\n"
> +			"lfdx     1,0,%4\n"
> +			"stfdx    1,0,%3\n"
> +			"sync\n"
> +			"lfdx     1,0,%2\n"
> +			"mtmsr    %0\n"
> +			"sync\n"
> +			"isync\n"
> +			: "=&r" (msr_save), "=&r" (msr)
> +			: "r" (&fp_save), "r" (&result), "r" (addr)
> +			: "memory" );
> +	local_irq_restore (flags);
> +
> +	return result;
> +}
> +
> +static inline void ___raw_writeq(unsigned long long value, int addr)
> +{
> +	unsigned long flags;
> +	unsigned long msr;
> +	unsigned long msr_save;
> +	unsigned long long fp_save;
> +
> +	local_irq_save (flags);
> +	asm volatile ("sync\n"
> +			"mfmsr    %0\n"
> +			"ori      %1,%0,0x2000\n"
> +			"mtmsr    %1\n"
> +			"isync\n"
> +			"stfdx    1,0,%2\n"
> +			"lfdx     1,0,%3\n"
> +			"stfdx    1,0,%4\n"
> +			"sync\n"
> +			"lfdx     1,0,%2\n"
> +			"mtmsr    %0\n"
> +			"sync\n"
> +			"isync\n"
> +			: "=&r" (msr_save), "=&r" (msr)
> +			: "r" (&fp_save), "r" (&value), "r" (addr)
> +			: "memory" );
> +	local_irq_restore (flags);
> +	return;
> +}
> +#define __raw_readq		___raw_readq
> +#define __raw_writeq		___raw_writeq
> +#else /* !CONFIG_PPC_FPU */
> +#define __raw_readq(addr)		({ BUG(); })
> +#define __raw_writeq(addr)		({ BUG(); })
> +#endif /* CONFIG_PPC_FPU */
> +
>  /*
>   * The insw/outsw/insl/outsl macros don't do byte-swapping.
>   * They are only used in practice for transferring buffers which
>


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

Re: Proposed changes to io.h

[Dan Malek]

Matt Porter wrote:


> ..... If e500 wanted a version they
> can use the SPE option to add some code.

This isn't an issue (yet) for e500.  The only 64-bit external bus is
for the DDR.  The only functions using the quad word are for MTD flash,
and all flash is on the 32-bit local bus.


	-- Dan


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

Re: Proposed changes to io.h

[Kumar Gala]

Agreed, however I dont remember how nice the assembler is (-me500) with
regards to accepting 'classic' FP instructions.  So, I'm in agreement
with Matt in that I'd prefer a build time option here.

- kumar

On Mar 31, 2004, at 4:52 PM, Dan Malek wrote:

>
> Matt Porter wrote:
>
>
>> ..... If e500 wanted a version they
>> can use the SPE option to add some code.
>
> This isn't an issue (yet) for e500.  The only 64-bit external bus is
> for the DDR.  The only functions using the quad word are for MTD flash,
> and all flash is on the 32-bit local bus.
>
>
> 	-- Dan
>
>


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

Re: Proposed changes to io.h

[Matt Porter]

On Wed, Mar 31, 2004 at 10:44:25AM -0500, John Whitney wrote:
> I've made few changes to include/asm-ppc/io.h that might want to be
> incorporated into the mainline tree.  These changes include:
>
> 1. Modifications to virt_to_bus, bus_to_virt, virt_to_phys, and
> phys_to_virt.  With the use of fully virtual addresses for
> cache-coherent allocations (consistent_alloc(), etc.), just subracting
> KERNELBASE from the virtual address is no longer sufficient.  Because
> of this, I have modified virt_to_phys and phys_to_virt to look like:

This group of calls is defined to only work on statically mapped
kernel virtual addresses. That is, the system memory mapped at
PAGE_OFFSET/KERNELBASE.  Adding any additional functionality to these
calls will just make ppc32 different from other arches. Yes, I know
the names are misleading but I didn't make them. :-P

If you want to drive a uniform address translation API into the
kernel then the place to start is on lkml.  This has been hashed
out on linuxppc-embedded before, please check the archives.

> 2. I'd like to add 64-bit __raw_readll and __raw_writell routines to
> io.h, done using floating-point registers.  Currently, modules such as
> MTD (when writing to 64-bit buses) perform two 32-bit, non-atomic
> writes, which can cause problems.  Using a floating-point register to
> guarantee a 64-bit write is ugly, but it works.  Code for these inlined
> routines is as follows:
>
> /*
>   * For reading and writing 64-bit values, we need to use the floating
> point
>   * registers.  The code will enable MSR_FP in the MSR register, use
> FPR1 to
>   * read from and write to memory, and then restore everything to the
>   * previous values.
>   */

I think this is useful...I've had to recommend this method for
local hacks on various people's platforms.  The most typical case
seems to be MTD but a lot of folks have custom data acquisition devices
that run into this problem. Yes, it's ugly, but hardware folks keep
configuring devices for a 64-bit bus width.

Why not create a patch against linux-2.5/linux-2.4 so that it is
easily reviewed?  Paul/Ben/others might then be compelled to comment
on it.

-Matt

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

Re: Proposed changes to io.h

[Dan Malek]

Matt Porter wrote:

 > ..... Yes, it's ugly, but hardware folks keep
> configuring devices for a 64-bit bus width.

We have to ensure we don't try to execute these functions on
processors that don't have FPUs.  I think all processors with
64-bit data bus have FPUs, so that may reduce the problem
just by being lucky.

Thanks.


	-- Dan


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

Re: Proposed changes to io.h

[Christoph Hellwig]

On Wed, Mar 31, 2004 at 10:44:25AM -0500, John Whitney wrote:
> 1. Modifications to virt_to_bus, bus_to_virt, virt_to_phys, and
> phys_to_virt.  With the use of fully virtual addresses for

These are all obsolete interface and you're not supposed to use them
for new plattforms or drivers at all.

>
> 2. I'd like to add 64-bit __raw_readll and __raw_writell routines to
> io.h, done using floating-point registers.  Currently, modules such as
> MTD (when writing to 64-bit buses) perform two 32-bit, non-atomic
> writes, which can cause problems.  Using a floating-point register to
> guarantee a 64-bit write is ugly, but it works.  Code for these inlined
> routines is as follows:

Standard-naming for those are readq/writeq afaik


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

Re: Proposed changes to io.h

[John Whitney]

>> 1. Modifications to virt_to_bus, bus_to_virt, virt_to_phys, and
>> phys_to_virt.  With the use of fully virtual addresses for
>
> These are all obsolete interface and you're not supposed to use them
> for new plattforms or drivers at all.

Just out of curiosity, are there replacements or are you just supposed
to do good-practice things like store the dma address returned by
pci_alloc_consistent/dma_alloc_coherent?

> Standard-naming for those are readq/writeq afaik

Specifically, the MTD code looks for __raw_readll/writell, which is why
I used the convention I did.  I'll be happy to update the names if
desired.

John


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

Re: Proposed changes to io.h

[Christoph Hellwig]

On Wed, Mar 31, 2004 at 01:40:15PM -0500, John Whitney wrote:
> Just out of curiosity, are there replacements or are you just supposed
> to do good-practice things like store the dma address returned by
> pci_alloc_consistent/dma_alloc_coherent?

The latter.  (or pci_map* / dma_map*)

> >Standard-naming for those are readq/writeq afaik
>
> Specifically, the MTD code looks for __raw_readll/writell, which is why
> I used the convention I did.  I'll be happy to update the names if
> desired.

What mtd code?

hch@bird:/repo/repo/linux-2.5$ grep __raw_readll include/asm-*/io.h | wc -l
0
hch@bird:/repo/repo/linux-2.5$ grep __raw_readq include/asm-*/io.h | wc -l
22


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

Re: Proposed changes to io.h

[John Whitney]

> What mtd code?
>
> hch@bird:/repo/repo/linux-2.5$ grep __raw_readll include/asm-*/io.h |
> wc -l
> 0
> hch@bird:/repo/repo/linux-2.5$ grep __raw_readq include/asm-*/io.h |
> wc -l
> 22
>
>

Specifically, include/linux/mtd/cfi.h and include/linux/mtd/map.h.  If
__raw_readll/writell aren't defined (as macros), they define them as a
direct read/write from a volatile u64 pointer.  I'll copy this over to
an MTD developer as well, perhaps we can fix MTD to use the standard
__raw_readq/writeq instead.

John


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

Re: Proposed changes to io.h

[John Whitney]

>> 1. Modifications to virt_to_bus, bus_to_virt, virt_to_phys, and
>> phys_to_virt.  With the use of fully virtual addresses for
>
> These are all obsolete interface and you're not supposed to use them
> for new plattforms or drivers at all.
>

pci_map_single() (include/asm-ppc/pci.h) and dma_map_single()
(include/asm-ppc/dma-mapping.h) use virt_to_bus().  Shouldn't some care
be taken to ensure that ANY virtual address passed to these routines
maps to the correct physical (bus) address?

John


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

Re: Proposed changes to io.h

[Dan Malek]

John Whitney wrote:

 > ..... Shouldn't some care
> be taken to ensure that ANY virtual address passed to these routines
> maps to the correct physical (bus) address?

Don't go there.  This has been discussed over and over in the past
and as Matt said, this is not the place.  Read the archives if
you want to see these discussions.

The code works according to the documentation, and the implementation
of the pci_*/dma_* functions allows the flexibility we need to support
systems where the mapping is more challenging.  The kernel and drivers
should not be directly using virt_to_bus() and all of its cousins, they
should use the DMA mapping functions documented.


	-- Dan


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

Re: Proposed changes to io.h

[Eugene Surovegin]

On Wed, Mar 31, 2004 at 04:09:13PM -0500, John Whitney wrote:
>
> >>1. Modifications to virt_to_bus, bus_to_virt, virt_to_phys, and
> >>phys_to_virt.  With the use of fully virtual addresses for
> >
> >These are all obsolete interface and you're not supposed to use them
> >for new plattforms or drivers at all.
> >
>
> pci_map_single() (include/asm-ppc/pci.h) and dma_map_single()
> (include/asm-ppc/dma-mapping.h) use virt_to_bus().  Shouldn't some care
> be taken to ensure that ANY virtual address passed to these routines
> maps to the correct physical (bus) address?
>

Please, read DMA-mapping.txt, it's quite clear on what memory is considered
DMA'able.

Eugene.

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

Re: Proposed changes to io.h

[John Whitney]

> Please, read DMA-mapping.txt, it's quite clear on what memory is
> considered
> DMA'able.
>

Sorry, Eugene.  It just seemed odd that dma_map_single(), called with
the virtual address produced by dma_alloc_coherent(), would return the
wrong address.  Thanks for the insight.

John


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

Re: Proposed changes to io.h

[Benjamin Herrenschmidt]

On Thu, 2004-04-01 at 01:44, John Whitney wrote:
> I've made few changes to include/asm-ppc/io.h that might want to be
> incorporated into the mainline tree.  These changes include:
>
> 1. Modifications to virt_to_bus, bus_to_virt, virt_to_phys, and
> phys_to_virt.  With the use of fully virtual addresses for
> cache-coherent allocations (consistent_alloc(), etc.), just subracting
> KERNELBASE from the virtual address is no longer sufficient.  Because
> of this, I have modified virt_to_phys and phys_to_virt to look like:

Those functions are deprecated actually (the _bus_ ones at least,
the _phys_ one can still be used by some internal arch code, though
it's well known that they will only work with the linear mapping,
thus a simple substraction is enough).

> This simplifies the "bus" routines, and makes sure that they use the
> standard virtual/physical translations.

The proper simplification is to kill them
>
> 2. I'd like to add 64-bit __raw_readll and __raw_writell routines to
> io.h, done using floating-point registers.  Currently, modules such as
> MTD (when writing to 64-bit buses) perform two 32-bit, non-atomic
> writes, which can cause problems.  Using a floating-point register to
> guarantee a 64-bit write is ugly, but it works.  Code for these inlined
> routines is as follows:

First, the proper name is readq/writeq ;) Then, most machines may
not have a 64 bits IO bus anyway, I don't think we need to provide
those functions for ppc32 and the MSR munging will cost you more
than the benefit of doing a 64 bits access.

So unless you have a specific need for those, I don't think we
need that in the kernel. Note about your implementation: you could
probably clear MSR:EE at the same time as you set MSR:FP instead
of using local_irq_save outside of the asm block ;)

Ben.


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

Re: Proposed changes to io.h

[Benjamin Herrenschmidt]

On Sat, 2004-04-03 at 01:41, John Whitney wrote:
> > No, you are not supposed to pass an address coming from
> > pci_alloc_consistent() to pci_map_single(), period.
> >
>
> Why take that stance, when a simple change to virt_to_phys() makes all
> virtual addresses coming from any routine work correctly?  Would that
> be far simpler system under which to implement code, with less prone to
> error (no need to worry about making sure you have the "correct"
> virtual memory type for a given routine - all virtual memory addresses
> would work).  This just sounds entirely political, and very little
> practical.

No. virt_to_phys() operates on the linear mapping only, that has
always been the case, doing otherwise would be non portable anyway.

I think we should deprecate virt_to_phys() anyway and use __pa in
the arch code, that would avoid people trying to use it for what it
is not.

Regarding pci_map_single(), it's semantics are clear regarding the
kind of memory it can map. Doing a ppc only hack to make the nonsense
work (passing it memory obtained by pci_alloc_consistent) would be
letting drivers writers do the wrong thing on ppc and break whenever
they port the driver to a platform that enforce that limitation.

We could add a runtime check to pci_map_single() to verify it gets
passed an address within the linear mapping though. That would catch
the bugs you are afraid of.

(Oh, and let's keep those discussions on-list).

Ben.


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

Re: Proposed changes to io.h

[John Whitney]

> We could add a runtime check to pci_map_single() to verify it gets
> passed an address within the linear mapping though. That would catch
> the bugs you are afraid of.

Might not be bad to add this as a conditional under one of the SANITY
type kernel hacking config options.

> (Oh, and let's keep those discussions on-list).
>

Sorry, Ben.  I'll shut up about it now, I promise. :-)

John


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