Re: [patch] Card services & yenta driver
This one...
- Fix some warnings which resulted from turning off
debug in cardbus.c
- sleep for the correct duration after taking the
reset away (this was left over from some testing. Sorry).
--- linux-2.4.0-test9-pre2/drivers/pcmcia/cardbus.c Mon Sep 18 20:31:49 2000
+++ linux-akpm/drivers/pcmcia/cardbus.c Tue Sep 19 00:08:23 2000
@@ -68,10 +68,9 @@
#include "cs_internal.h"
#include "rsrc_mgr.h"
-#ifndef PCMCIA_DEBUG
-#define PCMCIA_DEBUG 1
-#endif
+#ifdef PCMCIA_DEBUG
static int pc_debug = PCMCIA_DEBUG;
+#endif
/**/
@@ -372,9 +371,9 @@
void cb_enable(socket_info_t * s)
{
struct pci_dev *dev;
- u_char i, bus = s->cap.cb_dev->subordinate->number;
+ u_char i;
- DEBUG(0, "cs: cb_enable(bus %d)\n", bus);
+ DEBUG(0, "cs: cb_enable(bus %d)\n", s->cap.cb_dev->subordinate->number);
/* Configure bridge */
cb_release_cis_mem(s);
--- linux-2.4.0-test9-pre2/drivers/pcmcia/cs.c Mon Sep 18 20:31:49 2000
+++ linux-akpm/drivers/pcmcia/cs.c Tue Sep 19 00:08:33 2000
@@ -564,7 +564,7 @@
udelay((long)reset_time);
s->socket.flags &= ~SS_RESET;
set_socket(s, >socket);
-cs_sleep(unreset_delay/10);
+cs_sleep(unreset_delay);
unreset_socket(s);
} /* reset_socket */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Card services & yenta driver
Linus Torvalds wrote:
>
> ...
>
> I don't like Andrew's patch: I see why he does what he does, but it feels
> like papering over the real problem which is that the interfaces are just
> too opaque for the cs.c code to easily tell the controller what to do, and
> vice versa. They both have state, and neither of them can look at the
> others state, even though they are really talking about the same thing.
So the design objective is to keep the device driver as a thin,
stateless layer which simply converts generic messages and
indications into device-specific ones.
Shouldn't the per-socket threads then operate at the Card
Services layer, rather than at the driver layer? Sigh. I'm
not sure we really need the threads at all.
> I don't like this "pass hints around" approach. Why not say outright what
> you want done, instead of having magic flags that in combination with
> other magic flags imply what is going on?
OK, well I suggest that it is still logical for the Card Services layer
to send a message to the driver saying "the debounce period has
ended. Please flush any buffered insertion events". (Perhaps
it would be more logical to buffer the events at the CS layer...)
The previous implementation was a bit putrid, so this patch adds a
new method to pccard_operations and to pci_socket_ops called
"command". It's a generic "do something" interface which can
be expanded. I'm not sure if this is what you're after, but...
It also converts all the delay units to microseconds.
Linus, if you choose to not apply this (and who would blame
you after all this futzin'), please do apply the one in the
next email.
--- linux-2.4.0-test9-pre2/include/pcmcia/ss.h Mon Sep 18 20:31:50 2000
+++ linux-akpm/include/pcmcia/ss.h Mon Sep 18 22:54:17 2000
@@ -65,6 +65,9 @@
#define SS_CAP_PCCARD 0x4000
#define SS_CAP_CARDBUS 0x8000
+/* Commands to pci_socket_ops.command() */
+#define SC_DEBOUNCE_END1
+
/* for GetSocket, SetSocket */
typedef struct socket_state_t {
u_int flags;
@@ -82,7 +85,6 @@
#define SS_DMA_MODE0x0080
#define SS_SPKR_ENA0x0100
#define SS_OUTPUT_ENA 0x0200
-#define SS_DEBOUNCED 0x0400 /* Tell driver that the debounce delay has ended */
/* Flags for I/O port and memory windows */
#define MAP_ACTIVE 0x01
@@ -134,6 +136,7 @@
int (*get_mem_map)(unsigned int sock, struct pccard_mem_map *mem);
int (*set_mem_map)(unsigned int sock, struct pccard_mem_map *mem);
void (*proc_setup)(unsigned int sock, struct proc_dir_entry *base);
+ int (*command)(unsigned int sock, int cmd, void *arg);
};
/*
--- linux-2.4.0-test9-pre2/drivers/pcmcia/pci_socket.h Tue Jul 11 22:21:15 2000
+++ linux-akpm/drivers/pcmcia/pci_socket.h Mon Sep 18 22:59:13 2000
@@ -40,6 +40,7 @@
int (*get_mem_map)(struct pci_socket *, struct pccard_mem_map *);
int (*set_mem_map)(struct pci_socket *, struct pccard_mem_map *);
void (*proc_setup)(struct pci_socket *, struct proc_dir_entry *base);
+ int (*command)(struct pci_socket *, int cmd, void *arg);
};
extern struct pci_socket_ops yenta_operations;
--- linux-2.4.0-test9-pre2/drivers/pcmcia/ti113x.h Sat Aug 5 13:37:01 2000
+++ linux-akpm/drivers/pcmcia/ti113x.h Mon Sep 18 23:56:35 2000
@@ -189,7 +189,8 @@
yenta_set_io_map,
yenta_get_mem_map,
yenta_set_mem_map,
- yenta_proc_setup
+ yenta_proc_setup,
+ yenta_command,
};
#define ti_sysctl(socket) ((socket)->private[0])
@@ -232,7 +233,8 @@
yenta_set_io_map,
yenta_get_mem_map,
yenta_set_mem_map,
- yenta_proc_setup
+ yenta_proc_setup,
+ yenta_command,
};
#define ti_diag(socket)((socket)->private[0])
@@ -269,7 +271,8 @@
yenta_set_io_map,
yenta_get_mem_map,
yenta_set_mem_map,
- yenta_proc_setup
+ yenta_proc_setup,
+ yenta_command,
};
#endif /* CONFIG_CARDBUS */
--- linux-2.4.0-test9-pre2/drivers/pcmcia/ricoh.h Wed Jan 26 06:41:20 2000
+++ linux-akpm/drivers/pcmcia/ricoh.h Mon Sep 18 23:56:35 2000
@@ -157,7 +157,8 @@
yenta_set_io_map,
yenta_get_mem_map,
yenta_set_mem_map,
- yenta_proc_setup
+ yenta_proc_setup,
+ yenta_command,
};
#endif /* CONFIG_CARDBUS */
--- linux-2.4.0-test9-pre2/drivers/pcmcia/cardbus.c Mon Sep 18 20:31:49 2000
+++ linux-akpm/drivers/pcmcia/cardbus.c Mon Sep 18 23:03:47 2000
@@ -68,10 +68,9 @@
#include "cs_internal.h"
#include "rsrc_mgr.h"
-#ifndef PCMCIA_DEBUG
-#define PCMCIA_DEBUG 1
-#endif
+#ifdef PCMCIA_DEBUG
static int pc_debug = PCMCIA_DEBUG;
+#endif
/**/
@@ -372,9 +371,9 @@
void cb_enable(socket_info_t * s)
{
struct pci_dev *dev;
- u_char i, bus = s->cap.cb_dev->subordinate->number;
+ u_char i;
- DEBUG(0, "cs: cb_enable(bus %d)\n", bus);
+ DEBUG(0, "cs:
Re: [patch] Card services yenta driver
This one...
- Fix some warnings which resulted from turning off
debug in cardbus.c
- sleep for the correct duration after taking the
reset away (this was left over from some testing. Sorry).
--- linux-2.4.0-test9-pre2/drivers/pcmcia/cardbus.c Mon Sep 18 20:31:49 2000
+++ linux-akpm/drivers/pcmcia/cardbus.c Tue Sep 19 00:08:23 2000
@@ -68,10 +68,9 @@
#include "cs_internal.h"
#include "rsrc_mgr.h"
-#ifndef PCMCIA_DEBUG
-#define PCMCIA_DEBUG 1
-#endif
+#ifdef PCMCIA_DEBUG
static int pc_debug = PCMCIA_DEBUG;
+#endif
/**/
@@ -372,9 +371,9 @@
void cb_enable(socket_info_t * s)
{
struct pci_dev *dev;
- u_char i, bus = s-cap.cb_dev-subordinate-number;
+ u_char i;
- DEBUG(0, "cs: cb_enable(bus %d)\n", bus);
+ DEBUG(0, "cs: cb_enable(bus %d)\n", s-cap.cb_dev-subordinate-number);
/* Configure bridge */
cb_release_cis_mem(s);
--- linux-2.4.0-test9-pre2/drivers/pcmcia/cs.c Mon Sep 18 20:31:49 2000
+++ linux-akpm/drivers/pcmcia/cs.c Tue Sep 19 00:08:33 2000
@@ -564,7 +564,7 @@
udelay((long)reset_time);
s-socket.flags = ~SS_RESET;
set_socket(s, s-socket);
-cs_sleep(unreset_delay/10);
+cs_sleep(unreset_delay);
unreset_socket(s);
} /* reset_socket */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Card services & yenta driver
On Fri, 15 Sep 2000, David Hinds wrote:
> >
> > socket->events = 0;
> >
> > to "yenta_get_status()". Nothing more, nothing less.
>
> I think this is a bad idea. Ignoring latched events and relying on
> the current socket status is unsafe. You can ignore transient
> card-detect events when the driver hasn't yet started the power up
> sequence, but you cannot ignore them at other times, because the
> bridge does stuff (like cutting power, and redetecting card type) on
> its own.
Hmm.. You're probably right.
It still implies to me that the cs.c code is doing things at too low a
level - it does stuff at a controller level which means that the actual
low-level driver has a hard time trying to figure out what the CS level
really wants. For example, if the higher levels just had a "set power
state" thing, the low-level driver could know that the higher levels want
to change power, and more-over the low-level driver might be able to make
a decision based on that ("it's powering up the card, so I can get rid of
old events").
Right now, the low-level driver has no clue, it just has to assume that
any "set_socket()" call might be a power-up event, for example. It can
compare the new voltage to the one it has programmed in the controller,
but as Andrew pointed out that isn't actually even reliable - because
reading the power level is not going to actually tell you if the
controller had shut off power on its own for some reason.
I don't like Andrew's patch: I see why he does what he does, but it feels
like papering over the real problem which is that the interfaces are just
too opaque for the cs.c code to easily tell the controller what to do, and
vice versa. They both have state, and neither of them can look at the
others state, even though they are really talking about the same thing.
I don't like this "pass hints around" approach. Why not say outright what
you want done, instead of having magic flags that in combination with
other magic flags imply what is going on?
Actually, I'd love to just clear the events _after_ we have passed them
off to the cs layer (instead of before), but that would be inherently
racy, and we might end up missing a valid event if it came in at just the
right time. But a "acknowledge events" callback might work (ie the CS
layer would do a "acknowledge card present change" callback after it was
happy with the situation and had powered up the new card).
Or, we could just do the initial debouncing, all the SS_PENDING etc stuff
in the low-level driver. Which would side-step the communication issue
completely...
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Card services & yenta driver
On Fri, Sep 15, 2000 at 02:49:35PM -0700, Linus Torvalds wrote: > > The patch looks ok, but the SS_DEBOUNCE thing is horrible. > > Why do it? All of the SS_DETECT logic is inside cs.c anyway. Now you > introduce SS_DEBOUNCE to fix a cs.c bug, and "export" that cs.c logic bug > into the low-level driver. > > An alternative, and probably more logical, fix is much simpler: add a > simple > > socket->events = 0; > > to "yenta_get_status()". Nothing more, nothing less. I think this is a bad idea. Ignoring latched events and relying on the current socket status is unsafe. You can ignore transient card-detect events when the driver hasn't yet started the power up sequence, but you cannot ignore them at other times, because the bridge does stuff (like cutting power, and redetecting card type) on its own. -- Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Card services & yenta driver
On Wed, 13 Sep 2000, Andrew Morton wrote: > > Finally some clarity with what is going on inside this Dell CPx650 > laptop (TI PCI1225 Cardbus bridge). Yes, it _is_ contact bounce. It > seems to find the 3com NIC particularly offensive - the card can easily > bounce out 150 milliseconds after the first insertion interrupt. The patch looks ok, but the SS_DEBOUNCE thing is horrible. Why do it? All of the SS_DETECT logic is inside cs.c anyway. Now you introduce SS_DEBOUNCE to fix a cs.c bug, and "export" that cs.c logic bug into the low-level driver. An alternative, and probably more logical, fix is much simpler: add a simple socket->events = 0; to "yenta_get_status()". Nothing more, nothing less. Why? Because the whole point of the socket->handler() callback is to tell the cs layer that something new happened. But if the cs layer already did a "get_status()", then obviously the event is no longer "new". So it should be cleared out. Sensible? Also, I'd be a lot happier just changing all timeouts to microseconds. "centiseconds" really don't exist as a valid amount of time. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Card services yenta driver
On Wed, 13 Sep 2000, Andrew Morton wrote: Finally some clarity with what is going on inside this Dell CPx650 laptop (TI PCI1225 Cardbus bridge). Yes, it _is_ contact bounce. It seems to find the 3com NIC particularly offensive - the card can easily bounce out 150 milliseconds after the first insertion interrupt. The patch looks ok, but the SS_DEBOUNCE thing is horrible. Why do it? All of the SS_DETECT logic is inside cs.c anyway. Now you introduce SS_DEBOUNCE to fix a cs.c bug, and "export" that cs.c logic bug into the low-level driver. An alternative, and probably more logical, fix is much simpler: add a simple socket-events = 0; to "yenta_get_status()". Nothing more, nothing less. Why? Because the whole point of the socket-handler() callback is to tell the cs layer that something new happened. But if the cs layer already did a "get_status()", then obviously the event is no longer "new". So it should be cleared out. Sensible? Also, I'd be a lot happier just changing all timeouts to microseconds. "centiseconds" really don't exist as a valid amount of time. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Card services yenta driver
On Fri, Sep 15, 2000 at 02:49:35PM -0700, Linus Torvalds wrote: The patch looks ok, but the SS_DEBOUNCE thing is horrible. Why do it? All of the SS_DETECT logic is inside cs.c anyway. Now you introduce SS_DEBOUNCE to fix a cs.c bug, and "export" that cs.c logic bug into the low-level driver. An alternative, and probably more logical, fix is much simpler: add a simple socket-events = 0; to "yenta_get_status()". Nothing more, nothing less. I think this is a bad idea. Ignoring latched events and relying on the current socket status is unsafe. You can ignore transient card-detect events when the driver hasn't yet started the power up sequence, but you cannot ignore them at other times, because the bridge does stuff (like cutting power, and redetecting card type) on its own. -- Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Card services yenta driver
On Fri, 15 Sep 2000, David Hinds wrote:
socket-events = 0;
to "yenta_get_status()". Nothing more, nothing less.
I think this is a bad idea. Ignoring latched events and relying on
the current socket status is unsafe. You can ignore transient
card-detect events when the driver hasn't yet started the power up
sequence, but you cannot ignore them at other times, because the
bridge does stuff (like cutting power, and redetecting card type) on
its own.
Hmm.. You're probably right.
It still implies to me that the cs.c code is doing things at too low a
level - it does stuff at a controller level which means that the actual
low-level driver has a hard time trying to figure out what the CS level
really wants. For example, if the higher levels just had a "set power
state" thing, the low-level driver could know that the higher levels want
to change power, and more-over the low-level driver might be able to make
a decision based on that ("it's powering up the card, so I can get rid of
old events").
Right now, the low-level driver has no clue, it just has to assume that
any "set_socket()" call might be a power-up event, for example. It can
compare the new voltage to the one it has programmed in the controller,
but as Andrew pointed out that isn't actually even reliable - because
reading the power level is not going to actually tell you if the
controller had shut off power on its own for some reason.
I don't like Andrew's patch: I see why he does what he does, but it feels
like papering over the real problem which is that the interfaces are just
too opaque for the cs.c code to easily tell the controller what to do, and
vice versa. They both have state, and neither of them can look at the
others state, even though they are really talking about the same thing.
I don't like this "pass hints around" approach. Why not say outright what
you want done, instead of having magic flags that in combination with
other magic flags imply what is going on?
Actually, I'd love to just clear the events _after_ we have passed them
off to the cs layer (instead of before), but that would be inherently
racy, and we might end up missing a valid event if it came in at just the
right time. But a "acknowledge events" callback might work (ie the CS
layer would do a "acknowledge card present change" callback after it was
happy with the situation and had powered up the new card).
Or, we could just do the initial debouncing, all the SS_PENDING etc stuff
in the low-level driver. Which would side-step the communication issue
completely...
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
[patch] Card services & yenta driver
Finally some clarity with what is going on inside this Dell CPx650 laptop (TI PCI1225 Cardbus bridge). Yes, it _is_ contact bounce. It seems to find the 3com NIC particularly offensive - the card can easily bounce out 150 milliseconds after the first insertion interrupt. The user-visible effect of contact bounce is the message "cs: socket timed out during reset". What happens is that the insertion causes an interrupt, we wait for setup_delay to expire, then we apply power. The card becomes ready about 10 milliseconds later (according to CB_PWRCYCLE). Then the card bounces in and out of the socket. The card is auto-powered down and doesn't come back up. Hence the timeout. On to the patch: - During the conversion from timer-based to thread-based control, several of the magical delays in cs.c (setup_delay, resume_delay, shutdown_delay) got themselves accidentally reduced by a factor of ten. These have been set back to what they are in David's latest release. Everything is now in terms of centiseconds (1 cs = 1/100th of a second). Not a particularly user-friendly unit, but it preserves module parm compatibility with pcmcia-cs. There is one subtle change here: in pcmcia-cs the module parameter: options cs.o setup_delay=100 means to delay 100 jiffies. With this patch, it means 100 centiseconds. These are equivalent if HZ=100, but the delays in pcmcia_cs will vary inversely with variations in HZ. - Capriciously doubled the value of setup_delay to 10 centiseconds. - Changed the "socket timed out" message to tell people to try increasing the value of setup_delay. - Debouncing. This is a bug in the current implementation. When a card is inserted, we take an interrupt and the thread is scheduled. If, during the thread execution, the card bounces out and in (very likely), an additional interrupt is generated and socket->events:SS_DETECT is set. When parse_events returns, the thread sees the buffered insertion event and calls parse_events again. parse_events has no choice but to power down the card, kick out the driver, reload the driver, beep, beep, beep. There are a number of ways of addressing this, and a lot of them won't work. This patch treats `setup_delay' as the debounce period. When this delay expires it sends a message to the low-level driver telling it that the debounce period has ended, and that it should now forget any buffered insertion events which it had saved up. This was implemented via the new "one shot" flag socket_state_t.flags:SS_DEBOUNCED. Now, clearing events:SS_DETECT after the expiry of setup_delay begs the question "what if the card has really been ejected"? That's OK, because setup_socket checks this - if the card isn't there then it takes the "no card" path and everything is happy (I tested this). Finally, if we _do_ get contact bounce even after the expiry of setup_delay, we get a reset timeout and the card services state machine gives up. But there is now a buffered-up insertion event! So parse_events is again called and everything works out. BTW: It appears that these Cardbus controllers have protection circuitry which turns off the voltages when the card is unplugged. With this controller, the voltage remains off even when the card bounces back in again (fair enough). So the controller is in a state where the CB_SOCKET_CONTROL register has the value 0x33, but there is no voltage at the card. So this test in yenta_set_power(): if (reg != cb_readl(socket, CB_SOCKET_CONTROL)) cb_writel(socket, CB_SOCKET_CONTROL, reg); prevents us from powering the card back up. If the test is removed, then the act of writing 0x33 to a register which already has a value of 0x33 _does_ power the card up. But this is just FYI. I left the test in. --- linux-2.4.0-test8/include/pcmcia/ss.h Sat Sep 9 16:19:30 2000 +++ linux-akpm/include/pcmcia/ss.h Wed Sep 13 00:13:06 2000 @@ -82,6 +82,7 @@ #define SS_DMA_MODE0x0080 #define SS_SPKR_ENA0x0100 #define SS_OUTPUT_ENA 0x0200 +#define SS_DEBOUNCED 0x0400 /* Tell driver that the debounce delay has ended */ /* Flags for I/O port and memory windows */ #define MAP_ACTIVE 0x01 --- linux-2.4.0-test8/drivers/pcmcia/cardbus.c Sat Sep 9 16:19:26 2000 +++ linux-akpm/drivers/pcmcia/cardbus.c Wed Sep 13 00:13:06 2000 @@ -58,11 +58,6 @@ #include #include -#ifndef PCMCIA_DEBUG -#define PCMCIA_DEBUG 1 -#endif -static int pc_debug = PCMCIA_DEBUG; - #define IN_CARD_SERVICES #include #include @@ -72,6 +67,11 @@ #include #include "cs_internal.h" #include "rsrc_mgr.h" + +#ifndef PCMCIA_DEBUG +#define PCMCIA_DEBUG 1 +#endif +static int pc_debug = PCMCIA_DEBUG; /**/ --- linux-2.4.0-test8/drivers/pcmcia/cs.c Sat Sep 9 16:19:26 2000 +++ linux-akpm/drivers/pcmcia/cs.c Wed Sep 13 00:13:06 2000 @@
