diff mbox

ethtool: changes of emac_regs structure accordingly within driver emac_regs structure.

Message ID 20150521190917.6971eb09@fr-ThinkPad-W520
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Ivan Mikhaylov May 21, 2015, 3:09 p.m. UTC
In ibm_emac.c in ethtool size of emac structure which passing through to driver
is nailed down and not correlating with current emac_regs structure.

Signed-off-by: Ivan Mikhaylov <ivan@ru.ibm.com>
---
 ibm_emac.c |  143 +++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 107 insertions(+), 36 deletions(-)

Comments

Ben Hutchings May 31, 2015, 8:57 p.m. UTC | #1
On Thu, 2015-05-21 at 19:09 +0400, Ivan Mikhaylov wrote:
> In ibm_emac.c in ethtool size of emac structure which passing through to driver
> is nailed down and not correlating with current emac_regs structure.
> 
> Signed-off-by: Ivan Mikhaylov <ivan@ru.ibm.com>
[...]

This is not backward-compatible.  It ought to be possible to mix and
match old and new ethtool and driver, except for the EMAC4SYNC case
which has been broken up until now.

Using the new definition of struct emac_regs, I think the driver and
ethtool need to agree that the MAC register dump sizes are:

EMAC:      offsetof(struct emac_regs, u1)
EMAC4:     offsetof(struct emac_regs, u1.emac4) + sizeof(p->u1.emac4)
EMAC4SYNC: offsetof(struct emac_regs, u1.emac4sync) + sizeof(p->u1.emac4sync)

Ben.
Ivan Mikhaylov June 1, 2015, 12:30 p.m. UTC | #2
On Mon, 1 June 2015 12:57 +0400
Ben Hutchings <ben@decadent.org.uk> wrote:

>On Thu, 2015-05-21 at 19:09 +0400, Ivan Mikhaylov wrote:
>> In ibm_emac.c in ethtool size of emac structure which passing through
>> to driver is nailed down and not correlating with current emac_regs
>> structure.
>> 
>> Signed-off-by: Ivan Mikhaylov <ivan@ru.ibm.com>
>[...]
>
>This is not backward-compatible.  It ought to be possible to mix and
>match old and new ethtool and driver, except for the EMAC4SYNC case
>which has been broken up until now.
>
>Using the new definition of struct emac_regs, I think the driver and
>ethtool need to agree that the MAC register dump sizes are:
>
>EMAC:      offsetof(struct emac_regs, u1)
>EMAC4:     offsetof(struct emac_regs, u1.emac4) + sizeof(p->u1.emac4)
>EMAC4SYNC: offsetof(struct emac_regs, u1.emac4sync) +
>sizeof(p->u1.emac4sync)
>
>Ben.
>
>-- 
>Ben Hutchings
>Reality is just a crutch for people who can't handle science fiction.

Actually it is backward-compatible because we don't care about size
which is coming from driver side, only what we doing is map of driver
structure to ethtool structure and results will be same
for emac and emac4.

 struct emac_regs *p = (struct emac_regs *)(hdr + 1);

Also size which you mentioned (112 emac, 116 emac4) can be different
from what you saying cause this managed by dts files where we can set
something like 0x100 or 0x80 for this memory area and we will still
have problem in representing MII area if this size wasn't set right
in dts.

Ethtool will be work in same way even if we have emac or emac4.

Thank you for respond!

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings June 1, 2015, 6:11 p.m. UTC | #3
On Mon, 2015-06-01 at 16:30 +0400, Ivan Mikhaylov wrote:
> On Mon, 1 June 2015 12:57 +0400
> Ben Hutchings <ben@decadent.org.uk> wrote:
> 
> >On Thu, 2015-05-21 at 19:09 +0400, Ivan Mikhaylov wrote:
> >> In ibm_emac.c in ethtool size of emac structure which passing through
> >> to driver is nailed down and not correlating with current emac_regs
> >> structure.
> >> 
> >> Signed-off-by: Ivan Mikhaylov <ivan@ru.ibm.com>
> >[...]
> >
> >This is not backward-compatible.  It ought to be possible to mix and
> >match old and new ethtool and driver, except for the EMAC4SYNC case
> >which has been broken up until now.
> >
> >Using the new definition of struct emac_regs, I think the driver and
> >ethtool need to agree that the MAC register dump sizes are:
> >
> >EMAC:      offsetof(struct emac_regs, u1)
> >EMAC4:     offsetof(struct emac_regs, u1.emac4) + sizeof(p->u1.emac4)
> >EMAC4SYNC: offsetof(struct emac_regs, u1.emac4sync) +
> >sizeof(p->u1.emac4sync)
> >
> >Ben.
> >
> >-- 
> >Ben Hutchings
> >Reality is just a crutch for people who can't handle science fiction.
> 
> Actually it is backward-compatible because we don't care about size
> which is coming from driver side, only what we doing is map of driver
> structure to ethtool structure and results will be same
> for emac and emac4.
> 
>  struct emac_regs *p = (struct emac_regs *)(hdr + 1);

The following registers won't be printed correctly.

> Also size which you mentioned (112 emac, 116 emac4) can be different
> from what you saying cause this managed by dts files where we can set
> something like 0x100 or 0x80 for this memory area and we will still
> have problem in representing MII area if this size wasn't set right
> in dts.

Yes, I understand that.  However, the in-tree device trees consistently
use those as the resource sizes so I think ethtool used to work properly
for the machines supported by those.  Increasing the size of the MAC
register dump is a regression for them.

Ben.

> Ethtool will be work in same way even if we have emac or emac4.
> 
> Thank you for respond!
>
Ivan Mikhaylov June 3, 2015, 2:28 p.m. UTC | #4
On Mon, 1 Jun 2015 22:11:00 +0400
Ben Hutchings <ben@decadent.org.uk> wrote:

>On Mon, 2015-06-01 at 16:30 +0400, Ivan Mikhaylov wrote:
>> On Mon, 1 June 2015 12:57 +0400
>> Ben Hutchings <ben@decadent.org.uk> wrote:
>> 
>> >On Thu, 2015-05-21 at 19:09 +0400, Ivan Mikhaylov wrote:
>> >> In ibm_emac.c in ethtool size of emac structure which passing
>> >> through to driver is nailed down and not correlating with current
>> >> emac_regs structure.
>> >> 
>> >> Signed-off-by: Ivan Mikhaylov <ivan@ru.ibm.com>
>> >[...]
>> >
>> >This is not backward-compatible.  It ought to be possible to mix and
>> >match old and new ethtool and driver, except for the EMAC4SYNC case
>> >which has been broken up until now.
>> >
>> >Using the new definition of struct emac_regs, I think the driver and
>> >ethtool need to agree that the MAC register dump sizes are:
>> >
>> >EMAC:      offsetof(struct emac_regs, u1)
>> >EMAC4:     offsetof(struct emac_regs, u1.emac4) + sizeof(p->u1.emac4)
>> >EMAC4SYNC: offsetof(struct emac_regs, u1.emac4sync) +
>> >sizeof(p->u1.emac4sync)
>> >
>> >Ben.
>> >
>> >-- 
>> >Ben Hutchings
>> >Reality is just a crutch for people who can't handle science fiction.
>> 
>> Actually it is backward-compatible because we don't care about size
>> which is coming from driver side, only what we doing is map of driver
>> structure to ethtool structure and results will be same
>> for emac and emac4.
>> 
>>  struct emac_regs *p = (struct emac_regs *)(hdr + 1);
>
>The following registers won't be printed correctly.

But they will, how can I prove it? May be some examples you need?
I can show you 4 case for emac4sync that I've :
1. old ethtool old driver
2. patched ethtool old driver
3. patched ethtool patched driver
4. old ethtool patched driver

First 112 byte for emac and emac4 are same so part of this patch is wrong,
I admit it.

Should be something like :
+	} else if (hdr->version == EMAC4_VERSION || hdr->version == EMAC_VERSION) {
+		printf("IAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n",
+			p->u0.emac4.iaht1, p->u0.emac4.iaht2,
+			p->u0.emac4.iaht3, p->u0.emac4.iaht4);
+
+		printf("GAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n",
+			p->u0.emac4.gaht1, p->u0.emac4.gaht2,
+			p->u0.emac4.gaht3, p->u0.emac4.gaht4);
+
+		printf(" IPCR = 0x%08x\n\n", p->u1.emac4.ipcr);
 	}

Size of this struct equals 
struct emac_regs {
	u32 mr0;
	u32 mr1;
	u32 tmr0;
	u32 tmr1;
	u32 rmr;
	u32 isr;
	u32 iser;
	u32 iahr;
	u32 ialr;
	u32 vtpid;
	u32 vtci;
	u32 ptr;
	u32 iaht1;
	u32 iaht2;
	u32 iaht3;
	u32 iaht4;
	u32 gaht1;
	u32 gaht2;
	u32 gaht3;
	u32 gaht4;
	u32 lsah;
	u32 lsal;
	u32 ipgvr;
	u32 stacr;
	u32 trtr;
	u32 rwmr;
	u32 octx;
	u32 ocrx;
	u32 ipcr;
};

this struct 

struct emac_regs {
	/* Common registers across all EMAC implementations. */
	u32 mr0;			/* Special 	*/
	u32 mr1;			/* Reset 	*/
	u32 tmr0;			/* Special 	*/
	u32 tmr1;			/* Special 	*/
	u32 rmr;			/* Reset 	*/
	u32 isr;			/* Always 	*/
	u32 iser;			/* Reset 	*/
	u32 iahr;			/* Reset, R, T 	*/
	u32 ialr;			/* Reset, R, T 	*/
	u32 vtpid;			/* Reset, R, T 	*/
	u32 vtci;			/* Reset, R, T 	*/
	u32 ptr;			/* Reset,    T 	*/
	union {
		/* Registers unique to EMAC4 implementations */
		struct {
			u32 iaht1;	/* Reset, R	*/
			u32 iaht2;	/* Reset, R	*/
			u32 iaht3;	/* Reset, R	*/
			u32 iaht4;	/* Reset, R	*/
			u32 gaht1;	/* Reset, R	*/
			u32 gaht2;	/* Reset, R	*/
			u32 gaht3;	/* Reset, R	*/
			u32 gaht4;	/* Reset, R	*/
		} emac4;
		/* Registers unique to EMAC4SYNC implementations */
		//struct {
		//	u32 mahr;	/* Reset, R, T  */
		//	u32 malr;	/* Reset, R, T  */
		//	u32 mmahr;	/* Reset, R, T  */
		//	u32 mmalr;	/* Reset, R, T  */
		//	u32 rsvd0[4];
		//} emac4sync;
	} u0;
	/* Common registers across all EMAC implementations. */
	u32 lsah;
	u32 lsal;
	u32 ipgvr;			/* Reset,    T 	*/
	u32 stacr;			/* Special 	*/
	u32 trtr;			/* Special 	*/
	u32 rwmr;			/* Reset 	*/
	u32 octx;
	u32 ocrx;
	union {
		/* Registers unique to EMAC4 implementations */
		struct {
			u32 ipcr;
		} emac4;
		/* Registers unique to EMAC4SYNC implementations */
	//	struct {
	//		u32 rsvd1;
	//		u32 revid;
	//		u32 rsvd2[2];
	//		u32 iaht1;	/* Reset, R     */
	//		u32 iaht2;	/* Reset, R     */
	//		u32 iaht3;	/* Reset, R     */
	//		u32 iaht4;	/* Reset, R     */
	//		u32 iaht5;	/* Reset, R     */
	//		u32 iaht6;	/* Reset, R     */
	//		u32 iaht7;	/* Reset, R     */
	//		u32 iaht8;	/* Reset, R     */
	//		u32 gaht1;	/* Reset, R     */
	//		u32 gaht2;	/* Reset, R     */
	//		u32 gaht3;	/* Reset, R     */
	//		u32 gaht4;	/* Reset, R     */
	//		u32 gaht5;	/* Reset, R     */
	//		u32 gaht6;	/* Reset, R     */
	//		u32 gaht7;	/* Reset, R     */
	//		u32 gaht8;	/* Reset, R     */
	//		u32 tpc;	/* Reset, T     */
	//	} emac4sync;
	} u1;
};

Structs emac4 and emac4sync same on size for u0 union.
With emac4sync there is will be 196 because of emac4sync structure in union u1.
All other parts will be same as was even for emac/emac4. So with any updates
memory area for emac/emac4 still same... Even with old drivers for emac/emac4.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Mikhaylov June 23, 2015, 3:28 p.m. UTC | #5
On Wed, 3 Jun 2015 18:28:37 +0400
Ivan Mikhaylov <ivan@ru.ibm.com> wrote:

> On Mon, 1 Jun 2015 22:11:00 +0400
> Ben Hutchings <ben@decadent.org.uk> wrote:
> 
> >On Mon, 2015-06-01 at 16:30 +0400, Ivan Mikhaylov wrote:
> >> On Mon, 1 June 2015 12:57 +0400
> >> Ben Hutchings <ben@decadent.org.uk> wrote:
> >> 
> >> >On Thu, 2015-05-21 at 19:09 +0400, Ivan Mikhaylov wrote:
> >> >> In ibm_emac.c in ethtool size of emac structure which passing
> >> >> through to driver is nailed down and not correlating with
> >> >> current emac_regs structure.
> >> >> 
> >> >> Signed-off-by: Ivan Mikhaylov <ivan@ru.ibm.com>
> >> >[...]
> >> >
> >> >This is not backward-compatible.  It ought to be possible to mix
> >> >and match old and new ethtool and driver, except for the
> >> >EMAC4SYNC case which has been broken up until now.
> >> >
> >> >Using the new definition of struct emac_regs, I think the driver
> >> >and ethtool need to agree that the MAC register dump sizes are:
> >> >
> >> >EMAC:      offsetof(struct emac_regs, u1)
> >> >EMAC4:     offsetof(struct emac_regs, u1.emac4) +
> >> >sizeof(p->u1.emac4) EMAC4SYNC: offsetof(struct emac_regs,
> >> >u1.emac4sync) + sizeof(p->u1.emac4sync)
> >> >
> >> >Ben.
> >> >
> >> >-- 
> >> >Ben Hutchings
> >> >Reality is just a crutch for people who can't handle science
> >> >fiction.
> >> 
> >> Actually it is backward-compatible because we don't care about size
> >> which is coming from driver side, only what we doing is map of
> >> driver structure to ethtool structure and results will be same
> >> for emac and emac4.
> >> 
> >>  struct emac_regs *p = (struct emac_regs *)(hdr + 1);
> >
> >The following registers won't be printed correctly.
> 
> But they will, how can I prove it? May be some examples you need?
> I can show you 4 case for emac4sync that I've :
> 1. old ethtool old driver
> 2. patched ethtool old driver
> 3. patched ethtool patched driver
> 4. old ethtool patched driver
> 
> First 112 byte for emac and emac4 are same so part of this patch is
> wrong, I admit it.
> 
> Should be something like :
> +	} else if (hdr->version == EMAC4_VERSION || hdr->version ==
> EMAC_VERSION) {
> +		printf("IAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n",
> +			p->u0.emac4.iaht1, p->u0.emac4.iaht2,
> +			p->u0.emac4.iaht3, p->u0.emac4.iaht4);
> +
> +		printf("GAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n",
> +			p->u0.emac4.gaht1, p->u0.emac4.gaht2,
> +			p->u0.emac4.gaht3, p->u0.emac4.gaht4);
> +
> +		printf(" IPCR = 0x%08x\n\n", p->u1.emac4.ipcr);
>  	}
> 
> Size of this struct equals 
> struct emac_regs {
> 	u32 mr0;
> 	u32 mr1;
> 	u32 tmr0;
> 	u32 tmr1;
> 	u32 rmr;
> 	u32 isr;
> 	u32 iser;
> 	u32 iahr;
> 	u32 ialr;
> 	u32 vtpid;
> 	u32 vtci;
> 	u32 ptr;
> 	u32 iaht1;
> 	u32 iaht2;
> 	u32 iaht3;
> 	u32 iaht4;
> 	u32 gaht1;
> 	u32 gaht2;
> 	u32 gaht3;
> 	u32 gaht4;
> 	u32 lsah;
> 	u32 lsal;
> 	u32 ipgvr;
> 	u32 stacr;
> 	u32 trtr;
> 	u32 rwmr;
> 	u32 octx;
> 	u32 ocrx;
> 	u32 ipcr;
> };
> 
> this struct 
> 
> struct emac_regs {
> 	/* Common registers across all EMAC implementations. */
> 	u32 mr0;			/* Special 	*/
> 	u32 mr1;			/* Reset 	*/
> 	u32 tmr0;			/* Special 	*/
> 	u32 tmr1;			/* Special 	*/
> 	u32 rmr;			/* Reset 	*/
> 	u32 isr;			/* Always 	*/
> 	u32 iser;			/* Reset 	*/
> 	u32 iahr;			/* Reset, R, T 	*/
> 	u32 ialr;			/* Reset, R, T 	*/
> 	u32 vtpid;			/* Reset, R, T 	*/
> 	u32 vtci;			/* Reset, R, T 	*/
> 	u32 ptr;			/* Reset,    T 	*/
> 	union {
> 		/* Registers unique to EMAC4 implementations */
> 		struct {
> 			u32 iaht1;	/* Reset, R	*/
> 			u32 iaht2;	/* Reset, R	*/
> 			u32 iaht3;	/* Reset, R	*/
> 			u32 iaht4;	/* Reset, R	*/
> 			u32 gaht1;	/* Reset, R	*/
> 			u32 gaht2;	/* Reset, R	*/
> 			u32 gaht3;	/* Reset, R	*/
> 			u32 gaht4;	/* Reset, R	*/
> 		} emac4;
> 		/* Registers unique to EMAC4SYNC implementations */
> 		//struct {
> 		//	u32 mahr;	/* Reset, R, T  */
> 		//	u32 malr;	/* Reset, R, T  */
> 		//	u32 mmahr;	/* Reset, R, T  */
> 		//	u32 mmalr;	/* Reset, R, T  */
> 		//	u32 rsvd0[4];
> 		//} emac4sync;
> 	} u0;
> 	/* Common registers across all EMAC implementations. */
> 	u32 lsah;
> 	u32 lsal;
> 	u32 ipgvr;			/* Reset,    T 	*/
> 	u32 stacr;			/* Special 	*/
> 	u32 trtr;			/* Special 	*/
> 	u32 rwmr;			/* Reset 	*/
> 	u32 octx;
> 	u32 ocrx;
> 	union {
> 		/* Registers unique to EMAC4 implementations */
> 		struct {
> 			u32 ipcr;
> 		} emac4;
> 		/* Registers unique to EMAC4SYNC implementations */
> 	//	struct {
> 	//		u32 rsvd1;
> 	//		u32 revid;
> 	//		u32 rsvd2[2];
> 	//		u32 iaht1;	/* Reset, R     */
> 	//		u32 iaht2;	/* Reset, R     */
> 	//		u32 iaht3;	/* Reset, R     */
> 	//		u32 iaht4;	/* Reset, R     */
> 	//		u32 iaht5;	/* Reset, R     */
> 	//		u32 iaht6;	/* Reset, R     */
> 	//		u32 iaht7;	/* Reset, R     */
> 	//		u32 iaht8;	/* Reset, R     */
> 	//		u32 gaht1;	/* Reset, R     */
> 	//		u32 gaht2;	/* Reset, R     */
> 	//		u32 gaht3;	/* Reset, R     */
> 	//		u32 gaht4;	/* Reset, R     */
> 	//		u32 gaht5;	/* Reset, R     */
> 	//		u32 gaht6;	/* Reset, R     */
> 	//		u32 gaht7;	/* Reset, R     */
> 	//		u32 gaht8;	/* Reset, R     */
> 	//		u32 tpc;	/* Reset, T     */
> 	//	} emac4sync;
> 	} u1;
> };
> 
> Structs emac4 and emac4sync same on size for u0 union.
> With emac4sync there is will be 196 because of emac4sync structure in
> union u1. All other parts will be same as was even for emac/emac4. So
> with any updates memory area for emac/emac4 still same... Even with
> old drivers for emac/emac4.
> 
> 

Ben, sorry for molestation, there is no any response from you in 2 weeks.
Just to remind about my patchset, I have some questions about :
1. >>The following registers won't be printed correctly.
Which following registers? You didn't mention.
2. What should I do from your opinion for doing this patch better?
3. I'm still don't see any problem with backward compatibility cause
we just send bunch of data without size with this call and will lie 
for both parts in right place and why do you think it won't be able
to be backward?

David, may be you can help us to resolve that situation?

Thanks in advance.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/ibm_emac.c b/ibm_emac.c
index e128e48..3f3fb07 100644
--- a/ibm_emac.c
+++ b/ibm_emac.c
@@ -22,6 +22,10 @@ 
 #define EMAC_ETHTOOL_REGS_RGMII		0x00000002
 #define EMAC_ETHTOOL_REGS_TAH		0x00000004
 
+#define EMAC_VERSION			0
+#define EMAC4_VERSION			1
+#define EMAC4SYNC_VERSION		2
+
 struct emac_ethtool_regs_hdr {
 	u32 components;
 };
@@ -32,35 +36,78 @@  struct emac_ethtool_regs_subhdr {
 };
 
 struct emac_regs {
-	u32 mr0;
-	u32 mr1;
-	u32 tmr0;
-	u32 tmr1;
-	u32 rmr;
-	u32 isr;
-	u32 iser;
-	u32 iahr;
-	u32 ialr;
-	u32 vtpid;
-	u32 vtci;
-	u32 ptr;
-	u32 iaht1;
-	u32 iaht2;
-	u32 iaht3;
-	u32 iaht4;
-	u32 gaht1;
-	u32 gaht2;
-	u32 gaht3;
-	u32 gaht4;
+	/* Common registers across all EMAC implementations. */
+	u32 mr0;			/* Special 	*/
+	u32 mr1;			/* Reset 	*/
+	u32 tmr0;			/* Special 	*/
+	u32 tmr1;			/* Special 	*/
+	u32 rmr;			/* Reset 	*/
+	u32 isr;			/* Always 	*/
+	u32 iser;			/* Reset 	*/
+	u32 iahr;			/* Reset, R, T 	*/
+	u32 ialr;			/* Reset, R, T 	*/
+	u32 vtpid;			/* Reset, R, T 	*/
+	u32 vtci;			/* Reset, R, T 	*/
+	u32 ptr;			/* Reset,    T 	*/
+	union {
+		/* Registers unique to EMAC4 implementations */
+		struct {
+			u32 iaht1;	/* Reset, R	*/
+			u32 iaht2;	/* Reset, R	*/
+			u32 iaht3;	/* Reset, R	*/
+			u32 iaht4;	/* Reset, R	*/
+			u32 gaht1;	/* Reset, R	*/
+			u32 gaht2;	/* Reset, R	*/
+			u32 gaht3;	/* Reset, R	*/
+			u32 gaht4;	/* Reset, R	*/
+		} emac4;
+		/* Registers unique to EMAC4SYNC implementations */
+		struct {
+			u32 mahr;	/* Reset, R, T  */
+			u32 malr;	/* Reset, R, T  */
+			u32 mmahr;	/* Reset, R, T  */
+			u32 mmalr;	/* Reset, R, T  */
+			u32 rsvd0[4];
+		} emac4sync;
+	} u0;
+	/* Common registers across all EMAC implementations. */
 	u32 lsah;
 	u32 lsal;
-	u32 ipgvr;
-	u32 stacr;
-	u32 trtr;
-	u32 rwmr;
+	u32 ipgvr;			/* Reset,    T 	*/
+	u32 stacr;			/* Special 	*/
+	u32 trtr;			/* Special 	*/
+	u32 rwmr;			/* Reset 	*/
 	u32 octx;
 	u32 ocrx;
-	u32 ipcr;
+	union {
+		/* Registers unique to EMAC4 implementations */
+		struct {
+			u32 ipcr;
+		} emac4;
+		/* Registers unique to EMAC4SYNC implementations */
+		struct {
+			u32 rsvd1;
+			u32 revid;
+			u32 rsvd2[2];
+			u32 iaht1;	/* Reset, R     */
+			u32 iaht2;	/* Reset, R     */
+			u32 iaht3;	/* Reset, R     */
+			u32 iaht4;	/* Reset, R     */
+			u32 iaht5;	/* Reset, R     */
+			u32 iaht6;	/* Reset, R     */
+			u32 iaht7;	/* Reset, R     */
+			u32 iaht8;	/* Reset, R     */
+			u32 gaht1;	/* Reset, R     */
+			u32 gaht2;	/* Reset, R     */
+			u32 gaht3;	/* Reset, R     */
+			u32 gaht4;	/* Reset, R     */
+			u32 gaht5;	/* Reset, R     */
+			u32 gaht6;	/* Reset, R     */
+			u32 gaht7;	/* Reset, R     */
+			u32 gaht8;	/* Reset, R     */
+			u32 tpc;	/* Reset, T     */
+		} emac4sync;
+	} u1;
 };
 
 struct mal_regs {
@@ -121,8 +168,6 @@  static void *print_emac_regs(void *buf)
 	       "TRTR  = 0x%08x RWMR = 0x%08x\n"
 	       "IAR   = %04x%08x\n"
 	       "LSA   = %04x%08x\n"
-	       "IAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n"
-	       "GAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n"
 	       "VTPID = 0x%04x VTCI = 0x%04x\n"
 	       "IPGVR = 0x%04x STACR = 0x%08x\n"
 	       "OCTX  = 0x%08x OCRX = 0x%08x\n",
@@ -132,15 +177,41 @@  static void *print_emac_regs(void *buf)
 	       p->trtr, p->rwmr,
 	       p->iahr, p->ialr,
 	       p->lsah, p->lsal,
-	       p->iaht1, p->iaht2, p->iaht3, p->iaht4,
-	       p->gaht1, p->gaht2, p->gaht3, p->gaht4,
-	       p->vtpid, p->vtci, p->ipgvr, p->stacr, p->octx, p->ocrx);
-
-	if (hdr->version)
-		printf(" IPCR = 0x%08x\n\n", p->ipcr);
-	else {
-		printf("\n\n");
-		res -= sizeof(u32);
+	       p->vtpid, p->vtci,
+	       p->ipgvr, p->stacr, p->octx, p->ocrx);
+
+	if (hdr->version == EMAC4SYNC_VERSION) {
+		printf("MAHR  = 0x%08x MALR  = 0x%08x MMAHR = 0x%08x\n"
+			"MMALR  = 0x%08x REVID  = 0x%08x\n",
+			p->u0.emac4sync.mahr, p->u0.emac4sync.malr,
+			p->u0.emac4sync.mmahr, p->u0.emac4sync.mmalr,
+			p->u1.emac4sync.revid);
+
+		printf("IAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n",
+			p->u1.emac4sync.iaht1, p->u1.emac4sync.iaht2,
+			p->u1.emac4sync.iaht3, p->u1.emac4sync.iaht4);
+		printf("        0x%04x 0x%04x 0x%04x 0x%04x\n",
+			p->u1.emac4sync.iaht5, p->u1.emac4sync.iaht6,
+			p->u1.emac4sync.iaht7, p->u1.emac4sync.iaht8);
+
+
+		printf("GAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n",
+			p->u1.emac4sync.gaht1, p->u1.emac4sync.gaht2,
+			p->u1.emac4sync.gaht3, p->u1.emac4sync.gaht4);
+		printf("        0x%04x 0x%04x 0x%04x 0x%04x\n\n",
+			p->u1.emac4sync.gaht5, p->u1.emac4sync.gaht6,
+			p->u1.emac4sync.gaht7, p->u1.emac4sync.gaht8);
+
+	} else if (hdr->version == EMAC4_VERSION) {
+		printf("IAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n",
+			p->u0.emac4.iaht1, p->u0.emac4.iaht2,
+			p->u0.emac4.iaht3, p->u0.emac4.iaht4);
+
+		printf("GAHT  = 0x%04x 0x%04x 0x%04x 0x%04x\n",
+			p->u0.emac4.gaht1, p->u0.emac4.gaht2,
+			p->u0.emac4.gaht3, p->u0.emac4.gaht4);
+
+		printf(" IPCR = 0x%08x\n\n", p->u1.emac4.ipcr);
 	}
 	return res;
 }