MPC8xx PCMCIA driver

PATCH In review Marcelo Tosatti 6 participants 14 messages Aug 30, 2005 – Sep 14, 2005 Pcmcia

lore

Share & tools

lore

Marcelo Tosatti

author

2005-08-30 02:59 UTC #0 lore raw

PATCH 1/1 MPC8xx PCMCIA driver

Hi,

Here goes the 8xx PCMCIA driver, originally written by Magnus Damm, with several improvements and fixes.

The driver was merged in v2.4 but never forward ported to v2.6.

Please review, comments are welcome (including aesthetic enhancements).

Russell: The driver is using pccard_nonstatic_ops for card window management, even though the driver its marked SS_STATIC_MAP (using mem->static_map).

Otherwise card IO windows aren't allocated properly.

drivers/pcmcia/Kconfig +10 -0

@@ -154,6 +154,16 @@ config TCIC
 	  "Bridge" is the name used for the hardware inside your computer that
 	  PCMCIA cards are plugged into. If unsure, say N.
 
+config PCMCIA_M8XX
+        tristate "MPC8xx PCMCIA support"
+        depends on PCMCIA && PPC
+        select PCCARD_NONSTATIC
+        help
+        Say Y here to include support for PowerPC 8xx series PCMCIA 
+        controller.
+
+        This driver is also available as a module called m8xx_pcmcia.
+
 config HD64465_PCMCIA
 	tristate "HD64465 host bridge support"
 	depends on HD64465 && PCMCIA

drivers/pcmcia/Makefile +1 -0

@@ -25,6 +25,7 @@ obj-$(CONFIG_PD6729)				+= pd6729.o
 obj-$(CONFIG_I82365)				+= i82365.o
 obj-$(CONFIG_I82092)				+= i82092.o
 obj-$(CONFIG_TCIC)				+= tcic.o
+obj-$(CONFIG_PCMCIA_M8XX)			+= m8xx_pcmcia.o
 obj-$(CONFIG_HD64465_PCMCIA)			+= hd64465_ss.o
 obj-$(CONFIG_PCMCIA_SA1100)			+= sa11xx_core.o sa1100_cs.o
 obj-$(CONFIG_PCMCIA_SA1111)			+= sa11xx_core.o sa1111_cs.o

drivers/pcmcia/m8xx_pcmcia.c new file +1295 -0

1,295 lines not on this page · raw message

Marcelo Tosatti

author

2005-08-30 03:59 UTC #2 lore raw

Quoted 37 lines · Jeff Garzik

Quoted 36 lines · Marcelo Tosatti

drivers/pcmcia/m8xx_pcmcia.c new file +36 -0

+static int voltage_set(int slot, int vcc, int vpp)
+{
+	u_int reg = 0;
+
+	switch(vcc) {
+	case 0: break;
+	case 33:
+		reg |= BCSR1_PCVCTL4;
+		break;
+	case 50: 
+		reg |= BCSR1_PCVCTL5;
+		break;
+	default: 
+		return 1;
+	}
+
+	switch(vpp) {
+	case 0: break;
+	case 33: 
+	case 50:
+		if(vcc == vpp)
+			reg |= BCSR1_PCVCTL6;
+		else
+			return 1;
+		break;
+	case 120: 
+		reg |= BCSR1_PCVCTL7;
+	default:
+		return 1;
+	}
+
+	if(!((vcc == 50) || (vcc == 0)))
+		return 1;
+
+	/* first, turn off all power */
+

Quoted 8 lines

drivers/pcmcia/m8xx_pcmcia.c line 324
Quoted 6 lines
drivers/pcmcia/m8xx_pcmcia.c new file +6 -0
+	*((uint *)RPX_CSR_ADDR) &= ~(BCSR1_PCVCTL4 | BCSR1_PCVCTL5
+				     | BCSR1_PCVCTL6 | BCSR1_PCVCTL7);
+
+	/* enable new powersettings */
+
+	*((uint *)RPX_CSR_ADDR) |= reg;
Should use bus read/write functions, such as foo_readl() or iowrite32().

The memory map structure which contains device configuration/registers is _always_ directly mapped with pte's (the 8xx is a chip with builtin UART/network/etc functionality).

I don't think there is a need to use read/write acessors.

Quoted 2 lines

Don't use weird types in kernel code such as 'uint'. Use the more explicitly-sized u32.

OK, will fix the types and address the rest of your comments.

Thanks!

Paul Mackerras

2005-08-30 04:32 UTC #3 lore raw

Quoted 5 lines · Marcelo Tosatti

The memory map structure which contains device configuration/registers is _always_ directly mapped with pte's (the 8xx is a chip with builtin UART/network/etc functionality).
I don't think there is a need to use read/write acessors.

Generally on PowerPC you need to use at least the eieio instruction to prevent reordering of the loads and stores to the device. It's possible that 8xx is sufficiently in-order that you get away without putting in barrier instructions (eieio or sync), but it's not good practice to omit them.

You can use accessors such as in_be32 and in_le32 in this situation, when you have a kernel virtual address that is already mapped to the device.

Regards,
Paul.

Marcelo Tosatti

author

2005-08-30 17:09 UTC #4 lore raw

Hi Paul, Jeff,

Quoted 7 lines · Paul Mackerras

Quoted 5 lines · Marcelo Tosatti
The memory map structure which contains device configuration/registers is _always_ directly mapped with pte's (the 8xx is a chip with builtin UART/network/etc functionality).
I don't think there is a need to use read/write acessors.

Bullshit, yep :)

Quoted 5 lines

Generally on PowerPC you need to use at least the eieio instruction to prevent reordering of the loads and stores to the device. It's possible that 8xx is sufficiently in-order that you get away without putting in barrier instructions (eieio or sync), but it's not good practice to omit them.

On 8xx, guarded mappings seem to ensure synchronous operation of load/store instructions.

Since the internal memory map is guarded, eieio is redudant (ie thats why it gets away without explicit barriers now).

Quoted 1 line

From MPC860UM.pdf: 5.2.5.2.1 eieio Behavior

The purpose of eieio is to prevent loads and stores from executing speculatively when appropriate. This might be desirable for a FIFO, where performing a read or write changes the FIFO's data. This should not be done unless it is certain that the instruction will be completed and not cancelled. The same function as eieio can be accomplished by defining a memory space as having the guarded attribute in the MMU, in which case, the eieio instruction is redundant. However, eieio could be useful in the rare event that a region where speculative accesses are not allowed lies in the middle of a non-guarded page.

There is nothing which prevents compiler reordering though, as Jeff notes.

Quoted 3 lines

You can use accessors such as in_be32 and in_le32 in this situation, when you have a kernel virtual address that is already mapped to the device.

Do you think it would be worth to have lighterweight versions of in_be32/in_le32 functions (without eieio and isync) ? Would avoid the increase in code size and consequently cache footprint.

The IMMAP is referenced directly _all over_ the 8xx core code, must be fixed.

Quoted 20 lines

There are multiple reasons:
* Easier reviewing. One cannot easily distinguish between writing to normal kernel virtual memory and "magic" memory that produces magicaly side effects such as initiating DMA of a net packet.
* Compiler safety. As the code is written now, you have no guarantees that the compiler won't combine two stores to the same location, etc. Accessor macros are a convenient place to add compiler barriers or 'volatile' notations that the MPC8xx code lacks.
* Maintainable. foo_read[bwl] or foo_read{8,16,32} are preferred because that's the way other bus accessors look like -- yes even embedded SoC buses benefit from these code patterns. You want your driver to look like other drivers as much as possible.
* Convenience. The accessors can be a zero overhead memory read/write at a minimum. But they can also be convenient places to use special memory read/write instructions that specify uncached memop, compiler barriers, memory barriers, etc.

Jeff Garzik

2005-08-30 04:33 UTC #5 lore raw

In reply to Marcelo Tosatti

Quoted 50 lines · Marcelo Tosatti

Quoted 38 lines · Jeff Garzik

Quoted 36 lines · Marcelo Tosatti

drivers/pcmcia/m8xx_pcmcia.c new file +36 -0

+static int voltage_set(int slot, int vcc, int vpp)
+{
+	u_int reg = 0;
+
+	switch(vcc) {
+	case 0: break;
+	case 33:
+		reg |= BCSR1_PCVCTL4;
+		break;
+	case 50: 
+		reg |= BCSR1_PCVCTL5;
+		break;
+	default: 
+		return 1;
+	}
+
+	switch(vpp) {
+	case 0: break;
+	case 33: 
+	case 50:
+		if(vcc == vpp)
+			reg |= BCSR1_PCVCTL6;
+		else
+			return 1;
+		break;
+	case 120: 
+		reg |= BCSR1_PCVCTL7;
+	default:
+		return 1;
+	}
+
+	if(!((vcc == 50) || (vcc == 0)))
+		return 1;
+
+	/* first, turn off all power */
+

Quoted 8 lines

drivers/pcmcia/m8xx_pcmcia.c line 324
Quoted 6 lines
drivers/pcmcia/m8xx_pcmcia.c new file +6 -0
+	*((uint *)RPX_CSR_ADDR) &= ~(BCSR1_PCVCTL4 | BCSR1_PCVCTL5
+				     | BCSR1_PCVCTL6 | BCSR1_PCVCTL7);
+
+	/* enable new powersettings */
+
+	*((uint *)RPX_CSR_ADDR) |= reg;
Should use bus read/write functions, such as foo_readl() or iowrite32().

Quoted 5 lines

The memory map structure which contains device configuration/registers is _always_ directly mapped with pte's (the 8xx is a chip with builtin UART/network/etc functionality).
I don't think there is a need to use read/write acessors.

There are multiple reasons:

* Easier reviewing. One cannot easily distinguish between writing to normal kernel virtual memory and "magic" memory that produces magicaly side effects such as initiating DMA of a net packet.

* Compiler safety. As the code is written now, you have no guarantees that the compiler won't combine two stores to the same location, etc. Accessor macros are a convenient place to add compiler barriers or 'volatile' notations that the MPC8xx code lacks.

* Maintainable. foo_read[bwl] or foo_read{8,16,32} are preferred because that's the way other bus accessors look like -- yes even embedded SoC buses benefit from these code patterns. You want your driver to look like other drivers as much as possible.

* Convenience. The accessors can be a zero overhead memory read/write at a minimum. But they can also be convenient places to use special memory read/write instructions that specify uncached memop, compiler barriers, memory barriers, etc.

Regards,

Jeff

Russell King

2005-08-30 07:06 UTC #6 lore raw

In reply to Marcelo Tosatti

Quoted 2 lines · Marcelo Tosatti

Here goes the 8xx PCMCIA driver, originally written by Magnus Damm, with several improvements and fixes.

Please don't copy me on PCMCIA stuff - I don't look after PCMCIA anymore. Please use the linux-pcmcia mailing list on lists.infradead.org. Thanks.

Quoted 5 lines

Russell: The driver is using pccard_nonstatic_ops for card window management, even though the driver its marked SS_STATIC_MAP (using mem->static_map).
Otherwise card IO windows aren't allocated properly.

This seems very wrong. Are you suggesting that IO windows need to be dynamic and memory windows static? If so, the pcmcia code isn't designed to cope with that.

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Dominik Brodowski

maintainer

2005-09-01 08:53 UTC #7 lore raw

In reply to Marcelo Tosatti

Hi,

Quoted 3 lines · Marcelo Tosatti

Russell: The driver is using pccard_nonstatic_ops for card window management, even though the driver its marked SS_STATIC_MAP (using mem->static_map).

This is obviously broken. Where does it fail if pccard_static_ops is used?

drivers/pcmcia/m8xx_pcmcia.c line 222

Quoted 4 lines

drivers/pcmcia/m8xx_pcmcia.c new file +4 -0

+typedef struct  {
+	u_int regbit;
+	u_int eventbit;
+} event_table_t;

No typedefs, please.

Dominik

Magnus Damm

2005-09-01 11:44 UTC #8 lore raw

Hello all,

Nice to see that this driver gets forward ported to 2.6. I originally wrote it for pcmcia-cs, but it made its way into 2.4 after a while. Thanks to all the people who added code and fixes.

I'm not sure how the current Linux pcmcia layer works, and I am not involved in powerpc land anymore so I have no comments on the porting work or the driver itself.

Quoted 6 lines · Dominik Brodowski

Quoted 3 lines · Marcelo Tosatti
Russell: The driver is using pccard_nonstatic_ops for card window management, even though the driver its marked SS_STATIC_MAP (using mem->static_map).
This is obviously broken. Where does it fail if pccard_static_ops is used?

I remember it was interesting to write the driver for pcmcia-cs. This was because the mpc8xx socket hardware did not implement per-window offsets, and pcmcia-cs required that. So a wild guess is that this static/notstatic thing is related to that.

/ magnus

Marcelo Tosatti

author

2005-09-01 14:58 UTC #9 lore raw

In reply to Dominik Brodowski

Quoted 8 lines · Dominik Brodowski

Hi,
Quoted 3 lines · Marcelo Tosatti
Russell: The driver is using pccard_nonstatic_ops for card window management, even though the driver its marked SS_STATIC_MAP (using mem->static_map).
This is obviously broken. Where does it fail if pccard_static_ops is used?

IIRC pcmcia_request_io() fails to dynamically allocate IO windows for PCMCIA cards because find_io_region returns NULL.

OTOH, as Magnus noted, the memory windows are static:

* Because of the lacking offset register we must map the whole card.
* We assign each memory window PCMCIA_MEM_WIN_SIZE address space.
* Make sure there is (PCMCIA_MEM_WIN_SIZE * PCMCIA_MEM_WIN_NO
* * PCMCIA_SOCKETS_NO) bytes at PCMCIA_MEM_WIN_BASE.
* The i/o windows are dynamically allocated at PCMCIA_IO_WIN_BASE.
* They are maximum 64KByte each...

socket[i].socket.features = SS_CAP_PCCARD | SS_CAP_MEM_ALIGN | SS_CAP_STATIC_MAP; socket[i].socket.io_offset = 0;

Quoted 6 lines

drivers/pcmcia/m8xx_pcmcia.c line 222
Quoted 4 lines
drivers/pcmcia/m8xx_pcmcia.c new file +4 -0
+typedef struct  {
+	u_int regbit;
+	u_int eventbit;
+} event_table_t;
No typedefs, please.

OK, will fix.

Marcelo Tosatti

author

2005-09-14 14:17 UTC #10 lore raw

In reply to Dominik Brodowski

Quoted 6 lines · Dominik Brodowski

drivers/pcmcia/m8xx_pcmcia.c line 222
Quoted 4 lines
drivers/pcmcia/m8xx_pcmcia.c new file +4 -0
+typedef struct  {
+	u_int regbit;
+	u_int eventbit;
+} event_table_t;
No typedefs, please.

OK, I've converted it to a plain "struct".

I think this code is just following PCMCIA style:

typedef struct pccard_mem_map { 
	u_char	map;
	u_char	flags;
	u_short	speed;
	u_long	static_start;
	u_int	card_start;
	struct resource *res;
} pccard_mem_map;

Any reason why this typedef and similar ones in ss.h are wanted?

PCMCIA is also using u_xxx "weird data types" extensively.

Dominik Brodowski

maintainer

2005-09-14 14:27 UTC #11 lore raw

Quoted 10 lines · Marcelo Tosatti

Quoted 6 lines · Dominik Brodowski
drivers/pcmcia/m8xx_pcmcia.c line 222
Quoted 4 lines
drivers/pcmcia/m8xx_pcmcia.c new file +4 -0
+typedef struct  {
+	u_int regbit;
+	u_int eventbit;
+} event_table_t;
No typedefs, please.
OK, I've converted it to a plain "struct".

Thanks.

Quoted 11 lines

I think this code is just following PCMCIA style:

typedef struct pccard_mem_map { 
	u_char	map;
	u_char	flags;
	u_short	speed;
	u_long	static_start;
	u_int	card_start;
	struct resource *res;
} pccard_mem_map;

Quoted 2 lines

Any reason why this typedef and similar ones in ss.h are wanted?

There's no reason for this being typedef'ed, and it is contrary to the kernel source CodingStyle. However, all pcmcia code was external at first, that's why the CodingStyle differs.

Quoted 1 line

PCMCIA is also using u_xxx "weird data types" extensively.

Unfortunately, yes. However, I'm in the process of adapting it to the kernel CodingStyle. I don't change something from "u_int" to "unsigned int" just for the naming of it, though, but when I'm in the same area fixing one thing or another, I try to take care of it. New structs, like struct pcmcia_device, already adhere the normal kernel policy on structs and typedefs.

Thanks,
Dominik

Marcelo Tosatti

author

2005-09-14 18:28 UTC #12 lore raw

Here is an uptodated version of the MPC8xx PCMCIA driver for v2.6, addressing comments by Jeff and Dominik:

- use IO accessors instead of direct device memory referencing
- avoid usage of non-standard "uint/uchar" data types
- kill struct typedef's

Will submit it for inclusion once v2.6.14 is out.

Testing on 8xx platforms is more than welcome! Works like a charm on our custom hardware (CONFIG_PRxK).

drivers/pcmcia/Kconfig +10 -0

@@ -154,6 +154,16 @@ config TCIC
 	  "Bridge" is the name used for the hardware inside your computer that
 	  PCMCIA cards are plugged into. If unsure, say N.
 
+config PCMCIA_M8XX
+        tristate "MPC8xx PCMCIA support"
+        depends on PCMCIA && PPC
+        select PCCARD_NONSTATIC
+        help
+        Say Y here to include support for PowerPC 8xx series PCMCIA 
+        controller.
+
+        This driver is also available as a module called m8xx_pcmcia.
+
 config HD64465_PCMCIA
 	tristate "HD64465 host bridge support"
 	depends on HD64465 && PCMCIA

drivers/pcmcia/Makefile +1 -0

@@ -25,6 +25,7 @@ obj-$(CONFIG_PD6729)				+= pd6729.o
 obj-$(CONFIG_I82365)				+= i82365.o
 obj-$(CONFIG_I82092)				+= i82092.o
 obj-$(CONFIG_TCIC)				+= tcic.o
+obj-$(CONFIG_PCMCIA_M8XX)                              += m8xx_pcmcia.o
 obj-$(CONFIG_HD64465_PCMCIA)			+= hd64465_ss.o
 obj-$(CONFIG_PCMCIA_SA1100)			+= sa11xx_core.o sa1100_cs.o
 obj-$(CONFIG_PCMCIA_SA1111)			+= sa11xx_core.o sa1111_cs.o

drivers/pcmcia/m8xx_pcmcia.c new file +1290 -0

1,290 lines not on this page · raw message

Jeff Garzik

2005-09-14 18:46 UTC #13 lore raw

Quoted 10 lines · Marcelo Tosatti

Here is an uptodated version of the MPC8xx PCMCIA driver for v2.6, addressing comments by Jeff and Dominik:
- use IO accessors instead of direct device memory referencing
- avoid usage of non-standard "uint/uchar" data types
- kill struct typedef's
Will submit it for inclusion once v2.6.14 is out.
Testing on 8xx platforms is more than welcome! Works like a charm on our custom hardware (CONFIG_PRxK).

No complaints here...

Jeff