Davicom DM9000A on MPC5200B (powerpc) works using a dirty offsetting and byte trick

Davicom DM9000A on MPC5200B (powerpc) works using a dirty offsetting and byte trick

[Henk Stegeman]

Hello,

My board has a Davicom DM9000A ethernet controller on the MPC5200B's
Local Plus bus.
With the following setup and code the controller *works fine* with the
MPC5200B, but I think the code + setup to make it happen could be a
lot cleaner.

The DM9000A is setup for 16 bit mode, and wired as such to the
MPC5200B, A1 is wired to the DM9000 CMD pin.
The DM9000 is accessed trough two registers, address and data register.
The chip-select used is CS1 and it is setup in U-boot as follows:
/*	Start addres: FB000000
	Data Bus Size: 2 bytes
	Address Bus Size: 16 bit
	Mux off.
*/
#define CFG_CS1_CFG = 0x04041500

The resulting physical DM9000 register map is:
FB000000 : address register
FB000002 : data register

I have the following definition for this network device in my dts:

	enet1:ethernet@fb000000 {
		#size-cells = <1>;
		#address-cells = <1>;
		device_type = "network";
		compatible = "davicom,dm9000";
		reg = <0xfb000000 0x00000002 		// DM9000 Address register
			0xfb000002 0x00000002>;		// DM9000 Data register
		mac-address = [ 00 00 00 00 00 00 ]; // Filled in by u-boot
		interrupts = <1 1 0>;			// External interrupt 1
		interrupt-parent = <&mpc5200_pic>;
	};

To pass this information to the unmodified DM9000 driver I put
together a wrapper arch/powerpc/sysdev/dm9000_dev.c (see below) for
the of_ part (I reused most of the work from
arch/powerpc/sysdev/tsi108_dev.c):

Because the dm9000 driver uses the data address both as pointer to u16
and pointer to u8, this works for a little-endian cpu to the
little-endian dm9000, but not for the big endian MPC5200 to little
endian dm9000.
So I add an offset of 1 to this pointer when it is passed to the
dm9000 driver. This offset of 1 is wrong for the u16 accesses of the
DM9000 driver, besides that the data needs to be byteswapped for the
dm9000 driver.
For these two reasons I wrote the functions dm9000_outblk_16bit
dm9000_intblk_16bit dm9000_dumpblk_16bit which are passed via the
platform_data.

Apart from comments on my assumptions I have the following questions
about this situation and my code:
- Is it the right way to make a separate arch/powerpc/sysdev/*_dev.c
for reading the device-tree and passing it to a driver, in stead of
adding it to the driver itself?
- I think the best solution to handle the separate address and data
register is the 2 entry register array in the device tree as above,
this accounts for an odd connection to the DM9000's CMD pin. Agree?
- The MPC5200's chip-select can be configured to do byte-swapping on
read and write, however when I configured it as such and I removed my
offsetting by 1 and byte-swapping code It didn't work.
- Any suggestions to what could be wrong here? Or does the MPC5200 in
this case only byte swap u16 reads, but a u8 read is unchanged?
- What about how the DM9000 driver deals with this u8 read and u16
read, is this correct?

Thanks,

Henk

----

/*
 * dm9000 device setup code
 *
 * Maintained by Henk Stegeman < henk.stegeman@gmail.com >
 *
 * This program is free software; you can redistribute it and/or modify it
 * under  the terms of  the GNU General  Public License as published by the
 * Free Software Foundation;  either version 2 of the  License, or (at your
 * option) any later version.
 */

#include <linux/stddef.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/errno.h>
#include <linux/major.h>
#include <linux/delay.h>
#include <linux/irq.h>
#include <linux/module.h>
#include <linux/device.h>
#include <linux/platform_device.h>
#include <linux/dm9000.h>

#include <asm/system.h>
#include <asm/atomic.h>
#include <asm/io.h>
#include <asm/irq.h>
#include <asm/prom.h>
#include <mm/mmu_decl.h>

#ifdef DEBUG
#define DBG(fmt...) do { printk(fmt); } while(0)
#else
#define DBG(fmt...) do { } while(0)
#endif

#define U16_SWAP_BYTES(v) (((v << 8) & 0xff00) | ((v >> 8) & 0x00ff))
static void dm9000_outblk_16bit(void __iomem *reg, void *data, int count)
{
	u16 * pU16 = data;
	count = ( count + 1 ) >> 1;
	while (count > 0) {
		/* subtract 1 from reg to get back at the 16bit reg addr */
		out_be16(reg - 1, U16_SWAP_BYTES(* pU16));
		pU16 ++;
		count --;
	}
}

static void dm9000_inblk_16bit(void __iomem *reg, void *data, int count)
{
	u16 * pU16 = data;
	count = ( count + 1 ) >> 1;
	while (count > 0) {	
		/* subtract 1 from reg to get back at the 16bit reg addr */
		* pU16 = in_be16(reg - 1);		
		* pU16 = U16_SWAP_BYTES(* pU16);
		pU16 ++;
		count --;
	}
}

static void dm9000_dumpblk_16bit(void __iomem *reg, int count)
{
	volatile u16 tmp;
	count = ( count + 1 ) >> 1;
	while (count > 0) {	
		/* subtract 1 from reg to get back at the 16bit reg addr */
		tmp = in_be16(reg - 1);		
		count --;
	}
}

#define NUM_DM9000_RESOURCES 3
static int __init dm9000_eth_of_init(void)
{
	struct device_node *np;
	unsigned int i = 0;
	struct platform_device *dm9000_eth_dev;
	int ret;

	for_each_compatible_node(np, "network", "davicom,dm9000") {
		struct resource r[NUM_DM9000_RESOURCES];
		struct dm9000_plat_data dm9000_eth_data;
		const void *dev_addr;

		memset(r, 0, sizeof(r));
		memset(&dm9000_eth_data, 0, sizeof(dm9000_eth_data));

		/* Address register. */
		ret = of_address_to_resource(np, 0, &r[0]);
		++ r[0].start; /* dm9000 driver uses this as an address to a u8,
						  our address is to a big endian u16, add an
						  offset of 1 byte so dm9000 driver reads the expected
						  least-significant byte */
		DBG("%s: name:start->end = %s:0x%lx-> 0x%lx\n",
			__func__,r[0].name, r[0].start, r[0].end);
		if (ret)
			goto err;

		/* Data register. */
		ret = of_address_to_resource(np, 1, &r[1]);
		++ r[1].start; /* dm9000 driver uses this as an address to a u8,
						  our address is to a big endian u16, add an
						  offset of 1 byte so dm9000 driver reads the expected
						  least-significant byte */
		DBG("%s: name:start->end = %s:0x%lx-> 0x%lx\n",
			__func__,r[1].name, r[1].start, r[1].end);
		if (ret)
			goto err;

		/* IRQ. */
		r[2].name = "irq";
		r[2].start = irq_of_parse_and_map(np, 0);
		r[2].flags = IORESOURCE_IRQ_HIGHLEVEL;
		if (r[2].start == NO_IRQ) {
			DBG("%s: irq_of_parse_and_map failed\n", __func__);
		}

		r[2].end = irq_of_parse_and_map(np, 0);
		if (r[2].end == NO_IRQ) {
			DBG("%s: irq_of_parse_and_map failed\n", __func__);
		}
		r[2].flags = IORESOURCE_IRQ;
		DBG("%s: name:start->end = %s:0x%lx-> 0x%lx\n",
			__func__,r[2].name, r[2].start, r[2].end);

		dm9000_eth_dev =
		    platform_device_register_simple("dm9000", i++, &r[0],
						    NUM_DM9000_RESOURCES);

		if (IS_ERR(dm9000_eth_dev)) {
			ret = PTR_ERR(dm9000_eth_dev);
			goto err;
		}

		dev_addr = of_get_mac_address(np);
		if (dev_addr) {
			memcpy(dm9000_eth_data.dev_addr, dev_addr, 6);
			dm9000_eth_data.flags |= DM9000_PLATF_NO_EEPROM;
		}		
		dm9000_eth_data.flags |= DM9000_PLATF_16BITONLY;
		dm9000_eth_data.inblk = dm9000_inblk_16bit;
		dm9000_eth_data.outblk = dm9000_outblk_16bit;
		dm9000_eth_data.dumpblk = dm9000_dumpblk_16bit;

		ret =
		    platform_device_add_data(dm9000_eth_dev, &dm9000_eth_data,
					     sizeof(struct dm9000_plat_data));
		if (ret)
			goto unreg;
	}
	return 0;
unreg:
	platform_device_unregister(dm9000_eth_dev);
err:
	of_node_put(np);
	return ret;
}

arch_initcall(dm9000_eth_of_init);


----

Re: Davicom DM9000A on MPC5200B (powerpc) works using a dirty offsetting and byte trick

[Grant Likely]

On Fri, Feb 20, 2009 at 2:51 AM, Henk Stegeman <henk.stegeman@gmail.com> wr=
ote:
> I have the following definition for this network device in my dts:
>
> =A0 =A0 =A0 =A0enet1:ethernet@fb000000 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#size-cells =3D <1>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0#address-cells =3D <1>;

The ethernet node doesn't have any children, so drop the #size-cells
and #address-cells properties.

> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0device_type =3D "network";

Drop device_type too.  It only makes sense if you're running real OpenFirmw=
are.

> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "davicom,dm9000";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <0xfb000000 0x00000002 =A0 =A0 =A0=
 =A0 =A0 =A0// DM9000 Address register
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A00xfb000002 0x00000002>; =
=A0 =A0 =A0 =A0 // DM9000 Data register

Just make this reg =3D <0xfb000000 0x00000004>;.  Specify in the
documentation for the "davicom,dm9000" binding that the address
register is at offset 0 and the data register is at offset 2.

Actually, since this is the local plus bus, you should have this node
as a child of the localplus node.  Like this:

localbus {
    compatible =3D "fsl,mpc5200b-lpb", "fsl,mpc5200-lpb", "simple-bus";
    #address-cells =3D <2>;  // first cell is CS#, second is offset
    #size-cells =3D <1>;
    ranges =3D <1 0 0xfb000000 0x4>;  // CS#1, offset 0, mapped to
0xfb000000, size=3D4
    enet1: ethernet@fb000000 {
        compatible =3D "davicom,dm9000";
        reg =3D <0xfb000000 0x00000002            // DM9000 Address registe=
r
        mac-address =3D [ 00 00 00 00 00 00 ]; // Filled in by u-boot
        interrupts =3D <1 1 0>;                   // External interrupt 1
    };
};

Doing it this way provides drivers with the ability to get the chip
select number, which is important if you ever decide to use the
Localplus fifo to transfer to/from the device.

> To pass this information to the unmodified DM9000 driver I put
> together a wrapper arch/powerpc/sysdev/dm9000_dev.c (see below) for
> the of_ part (I reused most of the work from
> arch/powerpc/sysdev/tsi108_dev.c):
>
> Because the dm9000 driver uses the data address both as pointer to u16
> and pointer to u8, this works for a little-endian cpu to the
> little-endian dm9000, but not for the big endian MPC5200 to little
> endian dm9000.
> So I add an offset of 1 to this pointer when it is passed to the
> dm9000 driver. This offset of 1 is wrong for the u16 accesses of the
> DM9000 driver, besides that the data needs to be byteswapped for the
> dm9000 driver.

If the driver cannot handle big endian machines, then it is a driver
bug.  Don't be afraid to modify the driver to fix this and send a
patch.

> For these two reasons I wrote the functions dm9000_outblk_16bit
> dm9000_intblk_16bit dm9000_dumpblk_16bit which are passed via the
> platform_data.
>
> Apart from comments on my assumptions I have the following questions
> about this situation and my code:
> - Is it the right way to make a separate arch/powerpc/sysdev/*_dev.c
> for reading the device-tree and passing it to a driver, in stead of
> adding it to the driver itself?

Personally, I'd use the of_platform bus infrastructure to probe the
new device.  Register an of_platform_driver which will bind against
the device node for the ethernet device.  Then you have a choice:
option 1)  Your driver's probe method can either create a child
platform device which the original driver can bind against with the
correct pdata, or
option 2) your new driver can call into the original driver at a point
that bypasses the platform bus bindings (because they are handled by
the of_platform bus instead).

I typically choose option 2 because it requires less overhead and less
memory (one fewer probe call and one fewer struct device), but it will
probably require a little bit of refactoring the original driver to
provide call points to bypass the platform bus binding bit of the
driver.  I've done this many times, but it does depend on the driver
maintainer being okay with multiple bus bindings (platform and
of_platform) for a driver.  This is an ongoing debate.

See drivers/video/xilinxfb.c for an example of a driver with both
platform and of_platform bus bindings.  You'll notice at the end of
the file that both a platform driver and an of_platform driver are
getting registered.

> - I think the best solution to handle the separate address and data
> register is the 2 entry register array in the device tree as above,
> this accounts for an odd connection to the DM9000's CMD pin. Agree?

no.  If CMD could appear at a different offset, then define an
optional property (maybe cmd-reg-offset =3D <2>;) to handle the case
where CMD appears at a different offset.

> - The MPC5200's chip-select can be configured to do byte-swapping on
> read and write, however when I configured it as such and I removed my
> offsetting by 1 and byte-swapping code It didn't work.
> - Any suggestions to what could be wrong here? Or does the MPC5200 in
> this case only byte swap u16 reads, but a u8 read is unchanged?

This I don't know.  I'd have to play with it to figure it out.

> - What about how the DM9000 driver deals with this u8 read and u16
> read, is this correct?

Fix the driver.  It will result in a more useful driver for everyone
at the end of the day.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Davicom DM9000A on MPC5200B (powerpc) works using a dirty offsetting and byte trick

[Juergen Beisert]

Henk,

On Freitag, 20. Februar 2009, Henk Stegeman wrote:
> - Any suggestions to what could be wrong here? Or does the MPC5200 in
> this case only byte swap u16 reads, but a u8 read is unchanged?

You should not follow the Freescale bus signal names when you connect your 
external little endian device. Otherwise the offsets are always wrong.

Do it in this way instead:

    MPC          LE Device
  D[0..7]   <->  D[24..31]
  D[8..15]  <->  D[16..23]
  D[16..23] <->  D[8..15]
  D[24..31] <->  D[0..7]

If you connect your device in such a way, just enable CS's byte swap feature 
depending on your bus size and you are done (no additional software 
manipulation required). Now you can write bytes, words or longs and you will 
always write the correct data into the corresponding device register.

Hope it helps
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

Re: Davicom DM9000A on MPC5200B (powerpc) works using a dirty offsetting and byte trick

[Henk Stegeman]

Juergen,


I don't understand how this would work,

Now I do one byte-swap, which works.
 -I byteswap in software, for 16-bit cycles by byte swapping and for 8
bit cycles by adding an offset of 1.
 (The byte swapping on the chipselect is off)

Your advice includes two byteswaps, one by re-routing the data bus and
one by enabling the byte swap on the chip-select.
Or does one of them not really swap bytes?


Henk

On Sat, Mar 7, 2009 at 11:09 AM, Juergen Beisert <jbe@pengutronix.de> wrote=
:
> Henk,
>
> On Freitag, 20. Februar 2009, Henk Stegeman wrote:
>> - Any suggestions to what could be wrong here? Or does the MPC5200 in
>> this case only byte swap u16 reads, but a u8 read is unchanged?
>
> You should not follow the Freescale bus signal names when you connect you=
r
> external little endian device. Otherwise the offsets are always wrong.
>
> Do it in this way instead:
>
> =A0 =A0MPC =A0 =A0 =A0 =A0 =A0LE Device
> =A0D[0..7] =A0 <-> =A0D[24..31]
> =A0D[8..15] =A0<-> =A0D[16..23]
> =A0D[16..23] <-> =A0D[8..15]
> =A0D[24..31] <-> =A0D[0..7]
>
> If you connect your device in such a way, just enable CS's byte swap feat=
ure
> depending on your bus size and you are done (no additional software
> manipulation required). Now you can write bytes, words or longs and you w=
ill
> always write the correct data into the corresponding device register.
>
> Hope it helps
> Juergen
>
> --
> Pengutronix e.K. =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0| Juergen Beisert =A0 =A0 =A0 =A0 =A0 =A0 |
> Linux Solutions for Science and Industry =A0 =A0 =A0| Phone: +49-8766-939=
 228 =A0 =A0 |
> Vertretung Sued/Muenchen, Germany =A0 =A0 =A0 =A0 =A0 =A0 | Fax: =A0 +49-=
5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 =A0 =A0 =A0 =A0 =A0 =A0 =A0| http://www.=
pengutronix.de/ =A0|
>

Re: Davicom DM9000A on MPC5200B (powerpc) works using a dirty offsetting and byte trick

[Juergen Beisert]

Henk,

On Montag, 9. M=E4rz 2009, Henk Stegeman wrote:
> I don't understand how this would work,
>
> Now I do one byte-swap, which works.
>  -I byteswap in software, for 16-bit cycles by byte swapping and for 8
> bit cycles by adding an offset of 1.
>  (The byte swapping on the chipselect is off)
>
> Your advice includes two byteswaps, one by re-routing the data bus and
> one by enabling the byte swap on the chip-select.

My experience is the chip select byte swap feature only works correctly if =
you=20
connect a little endian device like I showed you.

> Or does one of them not really swap bytes?

Let me show you how it works. You must ensure you can write/read data in an=
y=20
data width, but at the side of the little endian device it always must be i=
n=20
the correct endianess. This example uses a 32 bit data width, but it works=
=20
for 16 bit, too.
=2D LE shows how a real litte endian CPU would write data
=2D MPC1 shows how MPC5200 will do it, without any byte swap and DO at the
  MPC5200 side is also D0 at the little endian device
=2D MPC2 shows how MPC5200 will do it, with D0 at the MPC5200 side is D24 a=
t the
  little endian device
=2D MPC3 shows how MPC5200 will do it, connected like MPC2 but also the chip
  select byte swap feature enabled
=2D LE DEV shows how the little endian device expects the data

You want to write this data at the given offset into the little endian devi=
ce:

 Bytes: 0:0x34, 1:0x12, 2:0x78, 3:0x56
 Worte:     0:0x1234       2:0x5678
 LONG:            0:0x56781234

Writing as bytes:

 Bytes: 0:0x34, 1:0x12, 2:0x78, 3:0x56

Offset  LE    MPC1  MPC2  MPC3  LE DEV
 0      0x34  0x56  0x34  0x34  0x34
 1      0x12  0x78  0x12  0x12  0x12
 2      0x78  0x12  0x78  0x78  0x78
 3      0x56  0x34  0x56  0x56  0x56
        ^^^^--------^^^^--^^^^--^^^^--> these are correct
              ^^^^--------------------> this is wrong

Writing as words:

 Words:     0:0x1234       2:0x5678

Offset   LE     MPC1  MPC2  MPC3  LE DEV
  0      0x34   0x78  0x12  0x34  0x34
 (1)     0x12   0x56  0x34  0x12  0x12
  2      0x78   0x34  0x56  0x78  0x78
 (3)     0x56   0x12  0x78  0x56  0x56
         ^^^^---------------^^^^--^^^^--> these are correct
                ^^^^--^^^^--------------> these are wrong

Writing as longs:

 LONG:         0:0x56781234

Offset   LE     MPC1  MPC2  MPC3  LE DEV
  0      0x34   0x34  0x56  0x34  0x34
 (1)     0x12   0x12  0x78  0x12  0x12
 (2)     0x78   0x78  0x12  0x78  0x78
 (3)     0x56   0x56  0x34  0x56  0x56
         ^^^^---^^^^ -------^^^^--^^^^--> these are correct
                      ^^^^--------------> this is wrong

So, the MPC3 example always writes correct data.

Hope it helps,
Juergen

=2D-=20
Pengutronix e.K.                              | Juergen Beisert            =
 |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228    =
 |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555=
 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/ =
 |