Re: svn commit: r244193 - head/sys/x86/include
On Fri, 14 Dec 2012, Carl Delsey wrote:
On 12/13/12 21:49, Bruce Evans wrote:
On Thu, 13 Dec 2012, Jim Harris wrote:
Log:
Add bus_space_read_8 and bus_space_write_8 for amd64.
Rather than trying to KASSERT for callers that invoke this on
IO tags, either do nothing (for write_8) or return ~0 (for read_8).
read_8 returns a uint64_t, so it cannot return the signed integer ~0.
It actually returns this signed integer converted to uint64_t. On
amd64, this is the 32 bit value 0x. The 64-bit value
0x should be returned.
I double checked and 0x is being returned when compiled by
both clang and gcc.
Oops. ~0 is (int)-1 (assuming 2's complement. This gets converted to
(uint64_t)-1 = 0xfff...fffUL.
I still prefer the explicit hex constant. Higher-level bus.h APIs and
clients mostly use ~0ul (with that spelling) for this. This is the
documented spelling in bus_alloc_resource(9). It is shorter and
more portable than 0xfff...fffUL where the number of f's is MD. The
ul in it avoids the sign extension problems that I was worried about
here. ~0 is unportable (depends on 2's complement). ~0u is portable
but even harder to use than ~0 in practice. It is what gives the
32-bit value unless it is actually ~0ul.
I wonder how these ~0ul's work for PAE -- on i386, they only give
32-bit values, but bus addresses are 64 bits for PAE. I guess they
just don't work for PAE. Nothing at all in the MI sys/bus.h supports
PAE's 64-bit bus addresses, since sys/bus.h uses u_long for almost
everything (int never uses [u]int64_t, [u]_quad_t or [unsigned] long
long).
Modified: head/sys/x86/include/bus.h
==
--- head/sys/x86/include/bus.hThu Dec 13 21:39:59 2012 (r244192)
+++ head/sys/x86/include/bus.hThu Dec 13 21:40:11 2012 (r244193)
@@ -130,6 +130,7 @@
#define BUS_SPACE_MAXADDR0x
#endif
This file spells the F in hex constants in upper case.
In the above definition and in previous ones, it is careful to spell out
the constants and not depend on sign extension. So it is also a style
bug to use ~0.
Are you saying it is a style bug to not match the style used above,
regardless of whether that style is right or wrong, or are you saying (~0)
is always a style bug?
You might as well match the above style.
~0 is at worst a style bug, not a wrong value like I said. It is harder to
use since its correctness depends on the arch being 2's complement, and
understanding its correctness depends on knowing C's promotion rules and
not forgetting them like I did.
I also don't mind using (~0ul) for most of the values, to be consistent
with the MI spelling. This might reduce ifdefs. But there is only 1 ifdef
for a 64-bit value now, and 1 would still be needed for the PAE case if
64 bits actually work for PAE.
Style bug visible in the above: space instead of tab after #define. This
style bugs is in all #define's near here, including the new one.
Type error in #define's just before the above: the above BUS_SPACE_MAXADDR
is for 32 bits. For amd64 and i386-PAE, BUS_SPACE_MAXADDR is of course
64 bits, but the ifdef tangle for it is not tangled enough to be correct:
BUS_SPACE_MAXADDR is 0xULL, on both, but bus_addr_t is only
unsigned long long on i386-PAE.
I think this should be a separate patch though, since it is unrelated to this
change.
Fine.
+#ifdef __amd64__
+static __inline uint64_t bus_space_read_8(bus_space_tag_t tag,
+ bus_space_handle_t handle,
+ bus_size_t offset);
+#endif
+
This is style-bug for bug compatible with the existing forward
declarations. Forward declarations of inline functions are nonsense.
They are from NetBSD, for K&R support. But FreeBSD never supported
K&R in bus-space headers, and the forward declarations never even
compiled with K&R, since they never used __P(()). Almost 1/3 of the
x86 bus.h consists of these negatively useful forward declarations.
Some of them are almost as large as the full functions, since they are
misformatted worse, with parameters starting at about column 40 instead
of about column 20, so so that many lines are needed just for the
parameters (to line them up in perfectly non-KNF gnu style).
Same with this - separate patch
Fine.
-#if 0/* Cause a link error for bus_space_read_8 */
-#definebus_space_read_8(t, h, o)!!! bus_space_read_8
unimplemented !!!
+#ifdef __amd64__
+static __inline uint64_t
+bus_space_read_8(bus_space_tag_t tag, bus_space_handle_t handle,
+ bus_size_t offset)
+{
+
+if (tag == X86_BUS_SPACE_IO) /* No 8 byte IO space access on x86 */
The comment is not indented, and should probably not be placed to the
right of the code. This file mostly doesn't place comments there, and
when it does it doesn't capitalize them. One that does also spells IO
as i/o.
All the #if 0 lines also start with an end of line comment, and they
Re: svn commit: r244193 - head/sys/x86/include
On 12/13/12 21:49, Bruce Evans wrote:
On Thu, 13 Dec 2012, Jim Harris wrote:
Log:
Add bus_space_read_8 and bus_space_write_8 for amd64.
Rather than trying to KASSERT for callers that invoke this on
IO tags, either do nothing (for write_8) or return ~0 (for read_8).
read_8 returns a uint64_t, so it cannot return the signed integer ~0.
It actually returns this signed integer converted to uint64_t. On
amd64, this is the 32 bit value 0x. The 64-bit value
0x should be returned.
I double checked and 0x is being returned when compiled
by both clang and gcc.
Using KASSERT here just makes bus.h too messy from both
polluting bus.h with systm.h (for any number of drivers that include
bus.h without first including systm.h) or ports that use bus.h
directly (i.e. libpciaccess) as reported by zeising@.
Also don't try to implement all of the other bus_space functions for
8 byte access since realistically only these two are needed for some
devices that expose 64-bit memory-mapped registers.
Good.
Put the amd64-specific functions here rather than
sys/amd64/include/bus.h
so that we can keep this header unified for x86, as requested by mdf@
and tijl@.
Not so good. I don't really like any of the unified headers.
Me neither, but it sounds like there is a good reason for it, which I'll
admit I don't fully understand. Sounds like it has something to do with
cross-compiling.
Modified: head/sys/x86/include/bus.h
==
--- head/sys/x86/include/bus.hThu Dec 13 21:39:59 2012 (r244192)
+++ head/sys/x86/include/bus.hThu Dec 13 21:40:11 2012 (r244193)
@@ -130,6 +130,7 @@
#define BUS_SPACE_MAXADDR0x
#endif
This file spells the F in hex constants in upper case.
In the above definition and in previous ones, it is careful to spell out
the constants and not depend on sign extension. So it is also a style
bug to use ~0.
Are you saying it is a style bug to not match the style used above,
regardless of whether that style is right or wrong, or are you saying
(~0) is always a style bug?
Style bug visible in the above: space instead of tab after #define. This
style bugs is in all #define's near here, including the new one.
Type error in #define's just before the above: the above
BUS_SPACE_MAXADDR
is for 32 bits. For amd64 and i386-PAE, BUS_SPACE_MAXADDR is of course
64 bits, but the ifdef tangle for it is not tangled enough to be correct:
BUS_SPACE_MAXADDR is 0xULL, on both, but bus_addr_t is
only
unsigned long long on i386-PAE.
I think this should be a separate patch though, since it is unrelated to
this change.
+#define BUS_SPACE_INVALID_DATA(~0)
#define BUS_SPACE_UNRESTRICTED(~0)
/*
@@ -221,6 +222,12 @@ static __inline u_int32_t bus_space_read
bus_space_handle_t handle,
bus_size_t offset);
+#ifdef __amd64__
+static __inline uint64_t bus_space_read_8(bus_space_tag_t tag,
+ bus_space_handle_t handle,
+ bus_size_t offset);
+#endif
+
This is style-bug for bug compatible with the existing forward
declarations. Forward declarations of inline functions are nonsense.
They are from NetBSD, for K&R support. But FreeBSD never supported
K&R in bus-space headers, and the forward declarations never even
compiled with K&R, since they never used __P(()). Almost 1/3 of the
x86 bus.h consists of these negatively useful forward declarations.
Some of them are almost as large as the full functions, since they are
misformatted worse, with parameters starting at about column 40 instead
of about column 20, so so that many lines are needed just for the
parameters (to line them up in perfectly non-KNF gnu style).
Same with this - separate patch
...
@@ -251,8 +258,16 @@ bus_space_read_4(bus_space_tag_t tag, bu
return (*(volatile u_int32_t *)(handle + offset));
}
-#if 0/* Cause a link error for bus_space_read_8 */
-#definebus_space_read_8(t, h, o)!!! bus_space_read_8
unimplemented !!!
+#ifdef __amd64__
+static __inline uint64_t
+bus_space_read_8(bus_space_tag_t tag, bus_space_handle_t handle,
+ bus_size_t offset)
+{
+
+if (tag == X86_BUS_SPACE_IO) /* No 8 byte IO space access on x86 */
The comment is not indented, and should probably not be placed to the
right of the code. This file mostly doesn't place comments there, and
when it does it doesn't capitalize them. One that does also spells IO
as i/o.
All the #if 0 lines also start with an end of line comment, and they are
all capitalized. By "not indented" are you saying that all end of line
comments must be preceded by a tab?
+return (BUS_SPACE_INVALID_DATA);
+return (*(volatile uint64_t *)(handle + offset));
+}
#endif
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn
Re: svn commit: r244193 - head/sys/x86/include
On Thu, 13 Dec 2012, Jim Harris wrote:
Log:
Add bus_space_read_8 and bus_space_write_8 for amd64.
Rather than trying to KASSERT for callers that invoke this on
IO tags, either do nothing (for write_8) or return ~0 (for read_8).
read_8 returns a uint64_t, so it cannot return the signed integer ~0.
It actually returns this signed integer converted to uint64_t. On
amd64, this is the 32 bit value 0x. The 64-bit value
0x should be returned.
Using KASSERT here just makes bus.h too messy from both
polluting bus.h with systm.h (for any number of drivers that include
bus.h without first including systm.h) or ports that use bus.h
directly (i.e. libpciaccess) as reported by zeising@.
Also don't try to implement all of the other bus_space functions for
8 byte access since realistically only these two are needed for some
devices that expose 64-bit memory-mapped registers.
Good.
Put the amd64-specific functions here rather than sys/amd64/include/bus.h
so that we can keep this header unified for x86, as requested by mdf@
and tijl@.
Not so good. I don't really like any of the unified headers.
Modified: head/sys/x86/include/bus.h
==
--- head/sys/x86/include/bus.h Thu Dec 13 21:39:59 2012(r244192)
+++ head/sys/x86/include/bus.h Thu Dec 13 21:40:11 2012(r244193)
@@ -130,6 +130,7 @@
#define BUS_SPACE_MAXADDR 0x
#endif
This file spells the F in hex constants in upper case.
In the above definition and in previous ones, it is careful to spell out
the constants and not depend on sign extension. So it is also a style
bug to use ~0.
Style bug visible in the above: space instead of tab after #define. This
style bugs is in all #define's near here, including the new one.
Type error in #define's just before the above: the above BUS_SPACE_MAXADDR
is for 32 bits. For amd64 and i386-PAE, BUS_SPACE_MAXADDR is of course
64 bits, but the ifdef tangle for it is not tangled enough to be correct:
BUS_SPACE_MAXADDR is 0xULL, on both, but bus_addr_t is only
unsigned long long on i386-PAE.
+#define BUS_SPACE_INVALID_DATA (~0)
#define BUS_SPACE_UNRESTRICTED (~0)
/*
@@ -221,6 +222,12 @@ static __inline u_int32_t bus_space_read
bus_space_handle_t handle,
bus_size_t offset);
+#ifdef __amd64__
+static __inline uint64_t bus_space_read_8(bus_space_tag_t tag,
+ bus_space_handle_t handle,
+ bus_size_t offset);
+#endif
+
This is style-bug for bug compatible with the existing forward
declarations. Forward declarations of inline functions are nonsense.
They are from NetBSD, for K&R support. But FreeBSD never supported
K&R in bus-space headers, and the forward declarations never even
compiled with K&R, since they never used __P(()). Almost 1/3 of the
x86 bus.h consists of these negatively useful forward declarations.
Some of them are almost as large as the full functions, since they are
misformatted worse, with parameters starting at about column 40 instead
of about column 20, so so that many lines are needed just for the
parameters (to line them up in perfectly non-KNF gnu style).
...
@@ -251,8 +258,16 @@ bus_space_read_4(bus_space_tag_t tag, bu
return (*(volatile u_int32_t *)(handle + offset));
}
-#if 0 /* Cause a link error for bus_space_read_8 */
-#definebus_space_read_8(t, h, o) !!! bus_space_read_8
unimplemented !!!
+#ifdef __amd64__
+static __inline uint64_t
+bus_space_read_8(bus_space_tag_t tag, bus_space_handle_t handle,
+bus_size_t offset)
+{
+
+ if (tag == X86_BUS_SPACE_IO) /* No 8 byte IO space access on x86 */
The comment is not indented, and should probably not be placed to the
right of the code. This file mostly doesn't place comments there, and
when it does it doesn't capitalize them. One that does also spells IO
as i/o.
+ return (BUS_SPACE_INVALID_DATA);
+ return (*(volatile uint64_t *)(handle + offset));
+}
#endif
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244193 - head/sys/x86/include
Author: jimharris
Date: Thu Dec 13 21:40:11 2012
New Revision: 244193
URL: http://svnweb.freebsd.org/changeset/base/244193
Log:
Add bus_space_read_8 and bus_space_write_8 for amd64.
Rather than trying to KASSERT for callers that invoke this on
IO tags, either do nothing (for write_8) or return ~0 (for read_8).
Using KASSERT here just makes bus.h too messy from both
polluting bus.h with systm.h (for any number of drivers that include
bus.h without first including systm.h) or ports that use bus.h
directly (i.e. libpciaccess) as reported by zeising@.
Also don't try to implement all of the other bus_space functions for
8 byte access since realistically only these two are needed for some
devices that expose 64-bit memory-mapped registers.
Put the amd64-specific functions here rather than sys/amd64/include/bus.h
so that we can keep this header unified for x86, as requested by mdf@
and tijl@.
Submitted by: Carl Delsey
MFC after:3 days
Modified:
head/sys/x86/include/bus.h
Modified: head/sys/x86/include/bus.h
==
--- head/sys/x86/include/bus.h Thu Dec 13 21:39:59 2012(r244192)
+++ head/sys/x86/include/bus.h Thu Dec 13 21:40:11 2012(r244193)
@@ -130,6 +130,7 @@
#define BUS_SPACE_MAXADDR 0x
#endif
+#define BUS_SPACE_INVALID_DATA (~0)
#define BUS_SPACE_UNRESTRICTED (~0)
/*
@@ -221,6 +222,12 @@ static __inline u_int32_t bus_space_read
bus_space_handle_t handle,
bus_size_t offset);
+#ifdef __amd64__
+static __inline uint64_t bus_space_read_8(bus_space_tag_t tag,
+ bus_space_handle_t handle,
+ bus_size_t offset);
+#endif
+
static __inline u_int8_t
bus_space_read_1(bus_space_tag_t tag, bus_space_handle_t handle,
bus_size_t offset)
@@ -251,8 +258,16 @@ bus_space_read_4(bus_space_tag_t tag, bu
return (*(volatile u_int32_t *)(handle + offset));
}
-#if 0 /* Cause a link error for bus_space_read_8 */
-#definebus_space_read_8(t, h, o) !!! bus_space_read_8
unimplemented !!!
+#ifdef __amd64__
+static __inline uint64_t
+bus_space_read_8(bus_space_tag_t tag, bus_space_handle_t handle,
+bus_size_t offset)
+{
+
+ if (tag == X86_BUS_SPACE_IO) /* No 8 byte IO space access on x86 */
+ return (BUS_SPACE_INVALID_DATA);
+ return (*(volatile uint64_t *)(handle + offset));
+}
#endif
/*
@@ -479,6 +494,12 @@ static __inline void bus_space_write_4(b
bus_space_handle_t bsh,
bus_size_t offset, u_int32_t value);
+#ifdef __amd64__
+static __inline void bus_space_write_8(bus_space_tag_t tag,
+ bus_space_handle_t bsh,
+ bus_size_t offset, uint64_t value);
+#endif
+
static __inline void
bus_space_write_1(bus_space_tag_t tag, bus_space_handle_t bsh,
bus_size_t offset, u_int8_t value)
@@ -512,8 +533,17 @@ bus_space_write_4(bus_space_tag_t tag, b
*(volatile u_int32_t *)(bsh + offset) = value;
}
-#if 0 /* Cause a link error for bus_space_write_8 */
-#definebus_space_write_8 !!! bus_space_write_8 not implemented
!!!
+#ifdef __amd64__
+static __inline void
+bus_space_write_8(bus_space_tag_t tag, bus_space_handle_t bsh,
+ bus_size_t offset, uint64_t value)
+{
+
+ if (tag == X86_BUS_SPACE_IO) /* No 8 byte IO space access on x86 */
+ return;
+ else
+ *(volatile uint64_t *)(bsh + offset) = value;
+}
#endif
/*
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
