struct cc770_priv *priv,
{
unsigned long base = (unsigned long)priv->reg_base;
- outb(reg, base);
- outb(val, base + 1);
+ outw( reg + ( val<< 8), base);
That modification does fix your problem, right? The others above don't
help nor harm but we don't know if it's really realted to the same
problem. I wll dig a bit deeper.
Exactly. The others above I removed because facing the opposite, even
missing interrupts but then just to avoid other possible side effects
and then assuming that they might be related.
OK. My concern: Can we be sure that 16bit accesses are always supported
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around the
8bit accesses already help?
About the "HM:" fixes, I did not find any info in the svn log. Maybe
Oliver knows why they have been added.
quoted
quoted
}
static int __devinit cc770_isa_probe(struct platform_device *pdev)
---------------------------------------------------------------------------------------------
Please let me know if this is OK for You, maybe You can do some tests as
well.
My board does not use indirect accesses.
Ah Ok, I see, but let me know if I may help with additional tests You would
like to have beside the other mentioned tests as long as I have the
hardware.
I will do some test anyway.
quoted
quoted
Would continue then with further tests regarding error conditions,
however
I realised another small issue with dropped packages at reception.
As soon as You read the first time from the socket and then You stop
reading
the packages are not counted as 'dropped' any more which is IMHO not
correct
because as soon as You stop reading they should be counted as dropped
again.
Will have a closer look now...
Thanks a lot.
Also I'll inform You as soon as I can continue with some further tests
regarding bus states and error counters and maybe also with patches in
case I have.
No hurry... I'm first awaiting some patches to go mainline.
Wolfgang.
That modification does fix your problem, right? The others above
don't
quoted
quoted
help nor harm but we don't know if it's really realted to the same
problem. I wll dig a bit deeper.
Exactly. The others above I removed because facing the opposite,
even
quoted
missing interrupts but then just to avoid other possible side
effects
quoted
and then assuming that they might be related.
OK. My concern: Can we be sure that 16bit accesses are always
supported
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
the
8bit accesses already help?
Hmmm... are there any register reads that need the
same 'double cycle' sequence ??
If so you need to stop reads being interleaved (with
themselves and writes) so requesting a 16bit access
doesn't help.
Which means you need a spinlock...
David
That modification does fix your problem, right? The others above
don't
quoted
quoted
quoted
help nor harm but we don't know if it's really realted to the same
problem. I wll dig a bit deeper.
Exactly. The others above I removed because facing the opposite,
even
quoted
quoted
missing interrupts but then just to avoid other possible side
effects
quoted
quoted
and then assuming that they might be related.
OK. My concern: Can we be sure that 16bit accesses are always
supported
quoted
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
the
quoted
8bit accesses already help?
Hmmm... are there any register reads that need the
same 'double cycle' sequence ??
If so you need to stop reads being interleaved (with
themselves and writes) so requesting a 16bit access
doesn't help.
Which means you need a spinlock...
David
@David: Thank You very much for that hint. You are right and to
implement correct we need a spinlock.
@Wolfgang: I was thinking about Your question regarding 8/16 bit and
in fact it wouldn't work at all on a clean 8 bit cards.
Further it wouldn't work on 16 bit cards where the MSB is not equal
to base port +1 and anyway, it's depending always on how the chip is
interfaced to the ISA bus and in which mode the chip is configured.
And therefore I was giving David's hint a try in using a spinlock in
function cc770_isa_port_write_reg_indirect() and patched as follows:
---------------------------------------------------------------------
@@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o)/* Nothing more to send, switch off interrupts */cc770_write_reg(priv,msgobj[mo].ctrl0,MSGVAL_RES|TXIE_RES|RXIE_RES|INTPND_RES);-/*-*WehadsomecasesofrepeatedIRQsomakesurethe-*INTisacknowledged-*/-cc770_write_reg(priv,msgobj[mo].ctrl0,-MSGVAL_UNC|TXIE_UNC|RXIE_UNC|INTPND_RES);stats->tx_packets++;can_get_echo_skb(dev,0);
The result is absolutely perfect. I was sending 2,000,000 telegrams
without any problem like the foregoing patch but now 8 bit compatible.
Further I have also investigated a bit the issue regarding the "HM"
patches.
I checked out the can4linux and I could find this patch in file
can_i82527funcs.c as mentioned by Oliver.
@Oliver: BTW thanks for Your investigation.
After studying the Makefile and some tests I discovered that
this file is just used for the target SBS_PC7 but funny enough not
for GENERIC_I82527.
I was seeking the declaration of CANout used in can_i82527funcs.c
to see if it's based on the same code we had and as well as in Lincan
but unfortunately the target SBS_PC7 doesn't compile with kernel
2.6.39-4 and moreover I got the message:
...can4linux/trunk/can4linux/i82527funcs.c:72:
error: implicit declaration of function 'CANout'
And additional not knowing if the board of SBS_PC7 is ISA or PCI based
I tried to find out and found after some research the following
thread:
http://old.nabble.com/-PATCH-v2--cc770-isa%3A-legacy-driver-for-CC770-i82725-on-the-ISA-bus-td24216347.html
According to that I assumed it's ISA based and is working with cc770_isa.
But not finding the declaration of CANout used in can_i82527funcs.c I
assumed further that it was done without spinlock which might cause
the effect of repeated or even lost interrupts as well depending on
the hardware configuration.
If this is the case then the HM patches would be obsolete and maybe
someone owning a SBS PC7 can test without these patches.
@Henrik: I would like to ask You if You can confirm this when You are
back.
Thanks a lot.
Wolfgang
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2012-01-09 23:11:44
On 01/09/2012 10:47 PM, Wolfgang Zarre wrote:
[...]
quoted hunk
quoted
quoted
OK. My concern: Can we be sure that 16bit accesses are always
supported
quoted
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
the
quoted
8bit accesses already help?
Hmmm... are there any register reads that need the
same 'double cycle' sequence ??
If so you need to stop reads being interleaved (with
themselves and writes) so requesting a 16bit access
doesn't help.
Which means you need a spinlock...
David
@David: Thank You very much for that hint. You are right and to
implement correct we need a spinlock.
@Wolfgang: I was thinking about Your question regarding 8/16 bit and
in fact it wouldn't work at all on a clean 8 bit cards.
Further it wouldn't work on 16 bit cards where the MSB is not equal
to base port +1 and anyway, it's depending always on how the chip is
interfaced to the ISA bus and in which mode the chip is configured.
And therefore I was giving David's hint a try in using a spinlock in
function cc770_isa_port_write_reg_indirect() and patched as follows:
---------------------------------------------------------------------
*skb, struct net_device *dev)
stats->tx_bytes += dlc;
-
- /*
- * HM: We had some cases of repeated IRQs so make sure the
- * INT is acknowledged I know it's already further up, but
- * doing again fixed the issue
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
return NETDEV_TX_OK;
}
*dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
stats->tx_packets++;
can_get_echo_skb(dev, 0);
diff --git a/drivers/net/can/cc770/cc770_isa.c
b/drivers/net/can/cc770/cc770_isa.c
index 4be5fe2..fe39eed 100644
Do we need a global or a per device spin lock? If this should be a per
device one, please introduce a cc770_isa_priv and put the spinlock
there. Don't forget to initialize the spinlock.
From: Wolfgang Grandegger <hidden> Date: 2012-01-10 09:31:31
On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote:
On 01/09/2012 10:47 PM, Wolfgang Zarre wrote:
[...]
quoted
quoted
quoted
OK. My concern: Can we be sure that 16bit accesses are always
supported
quoted
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
the
quoted
8bit accesses already help?
Hmmm... are there any register reads that need the
same 'double cycle' sequence ??
If so you need to stop reads being interleaved (with
themselves and writes) so requesting a 16bit access
doesn't help.
Which means you need a spinlock...
David
@David: Thank You very much for that hint. You are right and to
implement correct we need a spinlock.
@Wolfgang: I was thinking about Your question regarding 8/16 bit and
in fact it wouldn't work at all on a clean 8 bit cards.
Further it wouldn't work on 16 bit cards where the MSB is not equal
to base port +1 and anyway, it's depending always on how the chip is
interfaced to the ISA bus and in which mode the chip is configured.
And therefore I was giving David's hint a try in using a spinlock in
function cc770_isa_port_write_reg_indirect() and patched as follows:
---------------------------------------------------------------------
*skb, struct net_device *dev)
stats->tx_bytes += dlc;
-
- /*
- * HM: We had some cases of repeated IRQs so make sure the
- * INT is acknowledged I know it's already further up, but
- * doing again fixed the issue
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
return NETDEV_TX_OK;
}
*dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
Please provide an extra patch for these unrelated changes. If we really
want to remove it.
quoted
stats->tx_packets++;
can_get_echo_skb(dev, 0);
diff --git a/drivers/net/can/cc770/cc770_isa.c
b/drivers/net/can/cc770/cc770_isa.c
index 4be5fe2..fe39eed 100644
Do we need a global or a per device spin lock? If this should be a per
device one, please introduce a cc770_isa_priv and put the spinlock
there. Don't forget to initialize the spinlock.
Yes, that's what I was thinking as well but in the ocan driver I find:
/*
* we need a spinlock here, as the address register looks shared between
* two PC-ECAN devices. Moreover, we need to protect WRT interrupts
*/
Looks like wired hardware. Anyway, a global spinlock might be safer.
Wolfgang.
From: Henrik Maier <hidden> Date: 2012-01-10 11:41:56
Hello guys,
see comments below.
On 9/01/2012 10:47 PM, Wolfgang Zarre wrote:
Further I have also investigated a bit the issue regarding the "HM"
patches.
I checked out the can4linux and I could find this patch in file
can_i82527funcs.c as mentioned by Oliver.
@Oliver: BTW thanks for Your investigation.
After studying the Makefile and some tests I discovered that
this file is just used for the target SBS_PC7 but funny enough not
for GENERIC_I82527.
I was seeking the declaration of CANout used in can_i82527funcs.c
to see if it's based on the same code we had and as well as in Lincan
but unfortunately the target SBS_PC7 doesn't compile with kernel
2.6.39-4 and moreover I got the message:
...can4linux/trunk/can4linux/i82527funcs.c:72:
error: implicit declaration of function 'CANout'
And additional not knowing if the board of SBS_PC7 is ISA or PCI based
I tried to find out and found after some research the following
thread:
http://old.nabble.com/-PATCH-v2--cc770-isa%3A-legacy-driver-for-CC770-i82725-on-the-ISA-bus-td24216347.html
According to that I assumed it's ISA based and is working with cc770_isa.
But not finding the declaration of CANout used in can_i82527funcs.c I
assumed further that it was done without spinlock which might cause
the effect of repeated or even lost interrupts as well depending on
the hardware configuration.
If this is the case then the HM patches would be obsolete and maybe
someone owning a SBS PC7 can test without these patches.
@Henrik: I would like to ask You if You can confirm this when You are
back.
Thanks a lot.
Wolfgang
The can4linux port I made was done for a specific project and a specific
hardware, the SBS PC7, and only ever tested on that hardware. The i82527
code was already in the can4linux 3.x archive but not maintained for a
while and has become non-functional over time.
Initially we used the 2.4 Linux kernel and the can4linux version 3.1 as
basis for the re-integration of the i82527. I called our modified
version "3.1i" (i for Intel). This is from the changelog:
3.1i - Integration of the i82527 code into can4linux version 3.1,
major refactoring and clean-up of the i82527 code, fixed
interrupt lock-up issue w/ sending while receiving in heavy load,
fixed issue w/ stopping and re-starting the bus, added
new acceptance mask function to bring code in line with SJA1000 code
base, new I/O model to support the SJA1000 CANin/CANout macros,
support added to the makefile for generic i82527 cards and
SBS PC7compact DINrail mounted industry PC
by Henrik Maier of FOCUS Software Engineering Pty Ltd
<www.focus-sw.com>
As time passed the requirement for kernel 2.6 support emerged and we did
the same based on can4linux 3.3.2 and called it "3.3.2i" which later was
merged into the official can4linux archive and became 3.3.3.
The "repeated TX IRQ" issue came up only with kernel 2.6 and was not
required in the 2.4 version. The way I fixed it may not have been the
best, but it solved the immediate need of finishing a project and
getting the driver stable. I am not a kernel driver expert.
The PC7 hardware is now obsolete anyway. From my point we can do a
"clean" start and remove the old baggage.
The PC7 CAN interface is ISA based and I don't have a hardware any more
to test.
Some background info and our "i" versions are published here:
http://www.focus-sw.com/can/can4linux.html
I hope that helps a bit. It has been a few years since I worked with
can4linux but it is great to see that the community is further enhancing
CAN support for Linux.
Henrik
From: Wolfgang Grandegger <hidden> Date: 2012-01-10 11:59:55
Hi Henrik,
On 01/10/2012 12:41 PM, Henrik Maier wrote:
Hello guys,
see comments below.
On 9/01/2012 10:47 PM, Wolfgang Zarre wrote:
quoted
Further I have also investigated a bit the issue regarding the "HM"
patches.
I checked out the can4linux and I could find this patch in file
can_i82527funcs.c as mentioned by Oliver.
@Oliver: BTW thanks for Your investigation.
After studying the Makefile and some tests I discovered that
this file is just used for the target SBS_PC7 but funny enough not
for GENERIC_I82527.
I was seeking the declaration of CANout used in can_i82527funcs.c
to see if it's based on the same code we had and as well as in Lincan
but unfortunately the target SBS_PC7 doesn't compile with kernel
2.6.39-4 and moreover I got the message:
...can4linux/trunk/can4linux/i82527funcs.c:72:
error: implicit declaration of function 'CANout'
And additional not knowing if the board of SBS_PC7 is ISA or PCI based
I tried to find out and found after some research the following
thread:
http://old.nabble.com/-PATCH-v2--cc770-isa%3A-legacy-driver-for-CC770-i82725-on-the-ISA-bus-td24216347.html
According to that I assumed it's ISA based and is working with cc770_isa.
But not finding the declaration of CANout used in can_i82527funcs.c I
assumed further that it was done without spinlock which might cause
the effect of repeated or even lost interrupts as well depending on
the hardware configuration.
If this is the case then the HM patches would be obsolete and maybe
someone owning a SBS PC7 can test without these patches.
@Henrik: I would like to ask You if You can confirm this when You are
back.
Thanks a lot.
Wolfgang
The can4linux port I made was done for a specific project and a specific
hardware, the SBS PC7, and only ever tested on that hardware. The i82527
code was already in the can4linux 3.x archive but not maintained for a
while and has become non-functional over time.
Initially we used the 2.4 Linux kernel and the can4linux version 3.1 as
basis for the re-integration of the i82527. I called our modified
version "3.1i" (i for Intel). This is from the changelog:
3.1i - Integration of the i82527 code into can4linux version 3.1,
major refactoring and clean-up of the i82527 code, fixed
interrupt lock-up issue w/ sending while receiving in heavy load,
fixed issue w/ stopping and re-starting the bus, added
new acceptance mask function to bring code in line with SJA1000 code
base, new I/O model to support the SJA1000 CANin/CANout macros,
support added to the makefile for generic i82527 cards and
SBS PC7compact DINrail mounted industry PC
by Henrik Maier of FOCUS Software Engineering Pty Ltd
<www.focus-sw.com>
As time passed the requirement for kernel 2.6 support emerged and we did
the same based on can4linux 3.3.2 and called it "3.3.2i" which later was
merged into the official can4linux archive and became 3.3.3.
The "repeated TX IRQ" issue came up only with kernel 2.6 and was not
required in the 2.4 version. The way I fixed it may not have been the
best, but it solved the immediate need of finishing a project and
getting the driver stable. I am not a kernel driver expert.
OK, then most likely it's not a hardware issue.
The PC7 hardware is now obsolete anyway. From my point we can do a
"clean" start and remove the old baggage.
The PC7 CAN interface is ISA based and I don't have a hardware any more
to test.
Some background info and our "i" versions are published here:
http://www.focus-sw.com/can/can4linux.html
I hope that helps a bit. It has been a few years since I worked with
can4linux but it is great to see that the community is further enhancing
CAN support for Linux.
Yes, of course. Thanks for digging that out. As you suggest, we will
remove that code and wait what happens.
Wolfgang.
From: Wolfgang Zarre <hidden> Date: 2012-01-10 12:30:22
Hello Wolfgang,
On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote:
quoted
On 01/09/2012 10:47 PM, Wolfgang Zarre wrote:
[...]
quoted
quoted
quoted
OK. My concern: Can we be sure that 16bit accesses are always
supported
quoted
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
the
quoted
8bit accesses already help?
Hmmm... are there any register reads that need the
same 'double cycle' sequence ??
If so you need to stop reads being interleaved (with
themselves and writes) so requesting a 16bit access
doesn't help.
Which means you need a spinlock...
David
@David: Thank You very much for that hint. You are right and to
implement correct we need a spinlock.
@Wolfgang: I was thinking about Your question regarding 8/16 bit and
in fact it wouldn't work at all on a clean 8 bit cards.
Further it wouldn't work on 16 bit cards where the MSB is not equal
to base port +1 and anyway, it's depending always on how the chip is
interfaced to the ISA bus and in which mode the chip is configured.
And therefore I was giving David's hint a try in using a spinlock in
function cc770_isa_port_write_reg_indirect() and patched as follows:
---------------------------------------------------------------------
*skb, struct net_device *dev)
stats->tx_bytes += dlc;
-
- /*
- * HM: We had some cases of repeated IRQs so make sure the
- * INT is acknowledged I know it's already further up, but
- * doing again fixed the issue
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
return NETDEV_TX_OK;
}
*dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
Please provide an extra patch for these unrelated changes. If we really
want to remove it.
Sure, this I can do.
quoted
quoted
stats->tx_packets++;
can_get_echo_skb(dev, 0);
diff --git a/drivers/net/can/cc770/cc770_isa.c
b/drivers/net/can/cc770/cc770_isa.c
index 4be5fe2..fe39eed 100644
Do we need a global or a per device spin lock? If this should be a per
device one, please introduce a cc770_isa_priv and put the spinlock
there. Don't forget to initialize the spinlock.
Yes, that's what I was thinking as well but in the ocan driver I find:
/*
* we need a spinlock here, as the address register looks shared between
* two PC-ECAN devices. Moreover, we need to protect WRT interrupts
*/
Looks like wired hardware. Anyway, a global spinlock might be safer.
Hmmm, actually I thought to place the spinlock local because of having
the problem just with the interrupt and not with mutex.
But if global wouldn't it then better to make an array[MAX_DEV] for the
lock with initialisation in _init or _start?
But if PC-ECAN works with that configuration?
From: Wolfgang Zarre <hidden> Date: 2012-01-10 12:41:26
Hello David,
quoted
cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
+ unsigned long flags;
+ spin_lock_irqsave(&outb_lock, flags);
outb(reg, base);
outb(val, base + 1);
+ spin_unlock_irqrestore(&outb_lock, flags);
Is there a 'read_reg_indirect' function??
Yes, there is.
If so it also needs to use the same mutex.
Actually, I don't think that we have a problem with mutex
beside that it's using just one inb() statement but having
for sure with an interrupt between both outb() statements which
seems to be critical for the cc770.
However, if inb() or outb() can be interrupted then it would be
an issue.
I'd double check all references to the 'reg_base' field.
David
From: Wolfgang Zarre <hidden> Date: 2012-01-10 12:43:43
Hello guys,
Hi Henrik,
On 01/10/2012 12:41 PM, Henrik Maier wrote:
quoted
Hello guys,
see comments below.
On 9/01/2012 10:47 PM, Wolfgang Zarre wrote:
quoted
Further I have also investigated a bit the issue regarding the "HM"
patches.
I checked out the can4linux and I could find this patch in file
can_i82527funcs.c as mentioned by Oliver.
@Oliver: BTW thanks for Your investigation.
After studying the Makefile and some tests I discovered that
this file is just used for the target SBS_PC7 but funny enough not
for GENERIC_I82527.
I was seeking the declaration of CANout used in can_i82527funcs.c
to see if it's based on the same code we had and as well as in Lincan
but unfortunately the target SBS_PC7 doesn't compile with kernel
2.6.39-4 and moreover I got the message:
...can4linux/trunk/can4linux/i82527funcs.c:72:
error: implicit declaration of function 'CANout'
And additional not knowing if the board of SBS_PC7 is ISA or PCI based
I tried to find out and found after some research the following
thread:
http://old.nabble.com/-PATCH-v2--cc770-isa%3A-legacy-driver-for-CC770-i82725-on-the-ISA-bus-td24216347.html
According to that I assumed it's ISA based and is working with cc770_isa.
But not finding the declaration of CANout used in can_i82527funcs.c I
assumed further that it was done without spinlock which might cause
the effect of repeated or even lost interrupts as well depending on
the hardware configuration.
If this is the case then the HM patches would be obsolete and maybe
someone owning a SBS PC7 can test without these patches.
@Henrik: I would like to ask You if You can confirm this when You are
back.
Thanks a lot.
Wolfgang
The can4linux port I made was done for a specific project and a specific
hardware, the SBS PC7, and only ever tested on that hardware. The i82527
code was already in the can4linux 3.x archive but not maintained for a
while and has become non-functional over time.
Initially we used the 2.4 Linux kernel and the can4linux version 3.1 as
basis for the re-integration of the i82527. I called our modified
version "3.1i" (i for Intel). This is from the changelog:
3.1i - Integration of the i82527 code into can4linux version 3.1,
major refactoring and clean-up of the i82527 code, fixed
interrupt lock-up issue w/ sending while receiving in heavy load,
fixed issue w/ stopping and re-starting the bus, added
new acceptance mask function to bring code in line with SJA1000 code
base, new I/O model to support the SJA1000 CANin/CANout macros,
support added to the makefile for generic i82527 cards and
SBS PC7compact DINrail mounted industry PC
by Henrik Maier of FOCUS Software Engineering Pty Ltd
<www.focus-sw.com>
As time passed the requirement for kernel 2.6 support emerged and we did
the same based on can4linux 3.3.2 and called it "3.3.2i" which later was
merged into the official can4linux archive and became 3.3.3.
The "repeated TX IRQ" issue came up only with kernel 2.6 and was not
required in the 2.4 version. The way I fixed it may not have been the
best, but it solved the immediate need of finishing a project and
getting the driver stable. I am not a kernel driver expert.
OK, then most likely it's not a hardware issue.
quoted
The PC7 hardware is now obsolete anyway. From my point we can do a
"clean" start and remove the old baggage.
The PC7 CAN interface is ISA based and I don't have a hardware any more
to test.
Some background info and our "i" versions are published here:
http://www.focus-sw.com/can/can4linux.html
I hope that helps a bit. It has been a few years since I worked with
can4linux but it is great to see that the community is further enhancing
CAN support for Linux.
Yes, of course. Thanks for digging that out. As you suggest, we will
remove that code and wait what happens.
From: Wolfgang Grandegger <hidden> Date: 2012-01-10 14:20:29
On 01/10/2012 01:30 PM, Wolfgang Zarre wrote:
Hello Wolfgang,
quoted
On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote:
quoted
On 01/09/2012 10:47 PM, Wolfgang Zarre wrote:
[...]
quoted
quoted
quoted
OK. My concern: Can we be sure that 16bit accesses are always
supported
quoted
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
the
quoted
8bit accesses already help?
Hmmm... are there any register reads that need the
same 'double cycle' sequence ??
If so you need to stop reads being interleaved (with
themselves and writes) so requesting a 16bit access
doesn't help.
Which means you need a spinlock...
David
@David: Thank You very much for that hint. You are right and to
implement correct we need a spinlock.
@Wolfgang: I was thinking about Your question regarding 8/16 bit and
in fact it wouldn't work at all on a clean 8 bit cards.
Further it wouldn't work on 16 bit cards where the MSB is not equal
to base port +1 and anyway, it's depending always on how the chip is
interfaced to the ISA bus and in which mode the chip is configured.
And therefore I was giving David's hint a try in using a spinlock in
function cc770_isa_port_write_reg_indirect() and patched as follows:
---------------------------------------------------------------------
diff --git a/drivers/net/can/cc770/cc770.c
b/drivers/net/can/cc770/cc770.c
index 2d12f89..dad6707 100644
*skb, struct net_device *dev)
stats->tx_bytes += dlc;
-
- /*
- * HM: We had some cases of repeated IRQs so make sure the
- * INT is acknowledged I know it's already further up, but
- * doing again fixed the issue
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
return NETDEV_TX_OK;
}
*dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
Please provide an extra patch for these unrelated changes. If we really
want to remove it.
Sure, this I can do.
quoted
quoted
quoted
stats->tx_packets++;
can_get_echo_skb(dev, 0);
diff --git a/drivers/net/can/cc770/cc770_isa.c
b/drivers/net/can/cc770/cc770_isa.c
index 4be5fe2..fe39eed 100644
Please use a more specific name, e.g.: cc770_isa_port_lock
quoted
quoted
quoted
+
Do we need a global or a per device spin lock? If this should be a per
device one, please introduce a cc770_isa_priv and put the spinlock
there. Don't forget to initialize the spinlock.
Yes, that's what I was thinking as well but in the ocan driver I find:
/*
* we need a spinlock here, as the address register looks shared between
* two PC-ECAN devices. Moreover, we need to protect WRT interrupts
*/
Looks like wired hardware. Anyway, a global spinlock might be safer.
Hmmm, actually I thought to place the spinlock local because of having
the problem just with the interrupt and not with mutex.
But if global wouldn't it then better to make an array[MAX_DEV] for the
lock with initialisation in _init or _start?
Global means *one* spin-irq-lock for the indirect register access of all
devices. That might be the most efficient solution but we are sure that
it works, also with wired i82527 hardware, which seem to exist. That's
also what the related lincan and ocan drivers used.
Wolfgang.
From: Wolfgang Grandegger <hidden> Date: 2012-01-10 14:25:58
On 01/10/2012 03:20 PM, Wolfgang Grandegger wrote:
On 01/10/2012 01:30 PM, Wolfgang Zarre wrote:
quoted
Hello Wolfgang,
quoted
On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote:
quoted
On 01/09/2012 10:47 PM, Wolfgang Zarre wrote:
[...]
quoted
quoted
quoted
OK. My concern: Can we be sure that 16bit accesses are always
supported
quoted
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
the
quoted
8bit accesses already help?
Hmmm... are there any register reads that need the
same 'double cycle' sequence ??
If so you need to stop reads being interleaved (with
themselves and writes) so requesting a 16bit access
doesn't help.
Which means you need a spinlock...
David
@David: Thank You very much for that hint. You are right and to
implement correct we need a spinlock.
@Wolfgang: I was thinking about Your question regarding 8/16 bit and
in fact it wouldn't work at all on a clean 8 bit cards.
Further it wouldn't work on 16 bit cards where the MSB is not equal
to base port +1 and anyway, it's depending always on how the chip is
interfaced to the ISA bus and in which mode the chip is configured.
And therefore I was giving David's hint a try in using a spinlock in
function cc770_isa_port_write_reg_indirect() and patched as follows:
---------------------------------------------------------------------
diff --git a/drivers/net/can/cc770/cc770.c
b/drivers/net/can/cc770/cc770.c
index 2d12f89..dad6707 100644
*skb, struct net_device *dev)
stats->tx_bytes += dlc;
-
- /*
- * HM: We had some cases of repeated IRQs so make sure the
- * INT is acknowledged I know it's already further up, but
- * doing again fixed the issue
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
return NETDEV_TX_OK;
}
*dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
Please provide an extra patch for these unrelated changes. If we really
want to remove it.
Sure, this I can do.
quoted
quoted
quoted
stats->tx_packets++;
can_get_echo_skb(dev, 0);
diff --git a/drivers/net/can/cc770/cc770_isa.c
b/drivers/net/can/cc770/cc770_isa.c
index 4be5fe2..fe39eed 100644
Please use a more specific name, e.g.: cc770_isa_port_lock
quoted
quoted
quoted
quoted
+
Do we need a global or a per device spin lock? If this should be a per
device one, please introduce a cc770_isa_priv and put the spinlock
there. Don't forget to initialize the spinlock.
Yes, that's what I was thinking as well but in the ocan driver I find:
/*
* we need a spinlock here, as the address register looks shared between
* two PC-ECAN devices. Moreover, we need to protect WRT interrupts
*/
Looks like wired hardware. Anyway, a global spinlock might be safer.
Hmmm, actually I thought to place the spinlock local because of having
the problem just with the interrupt and not with mutex.
But if global wouldn't it then better to make an array[MAX_DEV] for the
lock with initialisation in _init or _start?
Global means *one* spin-irq-lock for the indirect register access of all
devices. That might be the most efficient solution but we are sure that
s/might/might not/
it works, also with wired i82527 hardware, which seem to exist. That's
also what the related lincan and ocan drivers used.
From: Wolfgang Grandegger <hidden> Date: 2012-01-10 14:43:52
On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
Hello David,
quoted
quoted
cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
+ unsigned long flags;
+ spin_lock_irqsave(&outb_lock, flags);
outb(reg, base);
outb(val, base + 1);
+ spin_unlock_irqrestore(&outb_lock, flags);
Is there a 'read_reg_indirect' function??
Yes, there is.
quoted
If so it also needs to use the same mutex.
Actually, I don't think that we have a problem with mutex
beside that it's using just one inb() statement but having
for sure with an interrupt between both outb() statements which
seems to be critical for the cc770.
But the indirect read function also sets the address register before
reading the data using inb(). This sequence should also not be
interrupted and therefore we need to synchronize. For the indirect
access of the SJA1000 we also need to add spinlocks. Wonder why nobody
complained so far.
Wolfgang.
From: Oliver Hartkopp <socketcan@hartkopp.net> Date: 2012-01-10 14:50:32
On 10.01.2012 15:43, Wolfgang Grandegger wrote:
On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
quoted
Hello David,
quoted
quoted
cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
+ unsigned long flags;
+ spin_lock_irqsave(&outb_lock, flags);
outb(reg, base);
outb(val, base + 1);
+ spin_unlock_irqrestore(&outb_lock, flags);
Is there a 'read_reg_indirect' function??
Yes, there is.
quoted
If so it also needs to use the same mutex.
Actually, I don't think that we have a problem with mutex
beside that it's using just one inb() statement but having
for sure with an interrupt between both outb() statements which
seems to be critical for the cc770.
But the indirect read function also sets the address register before
reading the data using inb(). This sequence should also not be
interrupted and therefore we need to synchronize. For the indirect
access of the SJA1000 we also need to add spinlocks. Wonder why nobody
complained so far.
Maybe due to old single core hardware that has SJA1000 hw with indirect
addressing? Or this old hardware has not been 'kernel-upgraded' so far ...
In any case you are right with the missing spinlocks.
Regards,
Oliver
From: Wolfgang Zarre <hidden> Date: 2012-01-10 16:13:58
Hello Wolfgang,
On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
quoted
Hello David,
quoted
quoted
cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
+ unsigned long flags;
+ spin_lock_irqsave(&outb_lock, flags);
outb(reg, base);
outb(val, base + 1);
+ spin_unlock_irqrestore(&outb_lock, flags);
Is there a 'read_reg_indirect' function??
Yes, there is.
quoted
If so it also needs to use the same mutex.
Actually, I don't think that we have a problem with mutex
beside that it's using just one inb() statement but having
for sure with an interrupt between both outb() statements which
seems to be critical for the cc770.
But the indirect read function also sets the address register before
reading the data using inb(). This sequence should also not be
interrupted and therefore we need to synchronize. For the indirect
access of the SJA1000 we also need to add spinlocks. Wonder why nobody
complained so far.
So, if I understand correct that means that inb() can be interrupted between
setting the address and reading. If this is the case then yes, we need
spinlock if this is not the case then IMHO we wouldn't need or am I wrong?
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2012-01-10 16:20:12
On 01/10/2012 05:13 PM, Wolfgang Zarre wrote:
Hello Wolfgang,
quoted
On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
quoted
Hello David,
quoted
quoted
cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
+ unsigned long flags;
+ spin_lock_irqsave(&outb_lock, flags);
outb(reg, base);
outb(val, base + 1);
+ spin_unlock_irqrestore(&outb_lock, flags);
Is there a 'read_reg_indirect' function??
Yes, there is.
quoted
If so it also needs to use the same mutex.
Actually, I don't think that we have a problem with mutex
beside that it's using just one inb() statement but having
for sure with an interrupt between both outb() statements which
seems to be critical for the cc770.
But the indirect read function also sets the address register before
reading the data using inb(). This sequence should also not be
interrupted and therefore we need to synchronize. For the indirect
access of the SJA1000 we also need to add spinlocks. Wonder why nobody
complained so far.
So, if I understand correct that means that inb() can be interrupted
between
setting the address and reading. If this is the case then yes, we need
spinlock if this is not the case then IMHO we wouldn't need or am I wrong?
If I understand correct, this function is the problem:
static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv,
int reg)
{
unsigned long base = (unsigned long)priv->reg_base;
outb(reg, base);
an interrupt can happen here, which does another indirect read or write
access, leaving "reg" pointing to the wrong register.
return inb(base + 1);
}
Mar
c
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
From: Wolfgang Grandegger <hidden> Date: 2012-01-10 16:24:18
Hi Wolfgang,
On 01/10/2012 05:13 PM, Wolfgang Zarre wrote:
Hello Wolfgang,
quoted
On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
quoted
Hello David,
quoted
quoted
cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
+ unsigned long flags;
+ spin_lock_irqsave(&outb_lock, flags);
outb(reg, base);
outb(val, base + 1);
+ spin_unlock_irqrestore(&outb_lock, flags);
Is there a 'read_reg_indirect' function??
Yes, there is.
quoted
If so it also needs to use the same mutex.
Actually, I don't think that we have a problem with mutex
beside that it's using just one inb() statement but having
for sure with an interrupt between both outb() statements which
seems to be critical for the cc770.
But the indirect read function also sets the address register before
reading the data using inb(). This sequence should also not be
interrupted and therefore we need to synchronize. For the indirect
access of the SJA1000 we also need to add spinlocks. Wonder why nobody
complained so far.
So, if I understand correct that means that inb() can be interrupted
between
setting the address and reading. If this is the case then yes, we need
spinlock if this is not the case then IMHO we wouldn't need or am I wrong?
I think we speak about different things. inb() cannot be interrupted but
outb() followed by inb(). For indirect accesses we need something like:
/* Spinlock for cc770_isa_port_write_reg_indirect */
static DEFINE_SPINLOCK(cc770_isa_port_lock);
static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv,
int reg)
{
unsigned long base = (unsigned long)priv->reg_base;
unsigned long flags;
u8 val;
spin_lock_irqsave(&cc770_isa_port_lock, flags);
outb(reg, base);
val = inb(base + 1);
spin_unlock_irqrestore(&cc770_isa_port_lock, flags);
return val;
}
static void cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
unsigned long flags;
spin_lock_irqsave(&cc770_isa_port_lock, flags);
outb(reg, base);
outb(val, base + 1);
spin_unlock_irqrestore(&cc770_isa_port_lock, flags);
}
Hope we are in synch now.
Wolfgang.
From: Wolfgang Zarre <hidden> Date: 2012-01-10 19:02:43
Hello Wolfgang,
-------- Original Message --------
Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527
From: Wolfgang Grandegger <redacted>
To: info@essax.com
Cc: David Laight <redacted>, Oliver Hartkopp <socketcan@hartkopp.net>, henrik@proconx.com, netdev@vger.kernel.org, linux-can@vger.kernel.org,
socketcan-users@lists.berlios.de, IreneV [off-list ref], Stanislav Yelenskiy [off-list ref], oe@port.de, henrik@focus-sw.com
Date: Tue Jan 10 2012 17:23:59 GMT+0100 (CET)
Hi Wolfgang,
On 01/10/2012 05:13 PM, Wolfgang Zarre wrote:
quoted
Hello Wolfgang,
quoted
On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
quoted
Hello David,
quoted
quoted
cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
+ unsigned long flags;
+ spin_lock_irqsave(&outb_lock, flags);
outb(reg, base);
outb(val, base + 1);
+ spin_unlock_irqrestore(&outb_lock, flags);
Is there a 'read_reg_indirect' function??
Yes, there is.
quoted
If so it also needs to use the same mutex.
Actually, I don't think that we have a problem with mutex
beside that it's using just one inb() statement but having
for sure with an interrupt between both outb() statements which
seems to be critical for the cc770.
But the indirect read function also sets the address register before
reading the data using inb(). This sequence should also not be
interrupted and therefore we need to synchronize. For the indirect
access of the SJA1000 we also need to add spinlocks. Wonder why nobody
complained so far.
So, if I understand correct that means that inb() can be interrupted
between
setting the address and reading. If this is the case then yes, we need
spinlock if this is not the case then IMHO we wouldn't need or am I wrong?
I think we speak about different things. inb() cannot be interrupted but
outb() followed by inb(). For indirect accesses we need something like:
/* Spinlock for cc770_isa_port_write_reg_indirect */
static DEFINE_SPINLOCK(cc770_isa_port_lock);
static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv,
int reg)
{
unsigned long base = (unsigned long)priv->reg_base;
unsigned long flags;
u8 val;
spin_lock_irqsave(&cc770_isa_port_lock, flags);
outb(reg, base);
val = inb(base + 1);
spin_unlock_irqrestore(&cc770_isa_port_lock, flags);
return val;
}
static void cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
unsigned long flags;
spin_lock_irqsave(&cc770_isa_port_lock, flags);
outb(reg, base);
outb(val, base + 1);
spin_unlock_irqrestore(&cc770_isa_port_lock, flags);
}
Hope we are in synch now.
Thanks a lot. Yes, now phase locked and in synch and sorry, by mistake
I looked at the wrong function (cc770_isa_port_read_reg) in the heat of
the moment.
Absolutely clear, there we need a spinlock definitely. I'll start another test run
just to confirm.
From: Wolfgang Zarre <hidden> Date: 2012-01-11 09:00:57
Hello Wolfgang,
Hello Wolfgang,
quoted
On 01/10/2012 12:11 AM, Marc Kleine-Budde wrote:
quoted
On 01/09/2012 10:47 PM, Wolfgang Zarre wrote:
[...]
quoted
quoted
quoted
OK. My concern: Can we be sure that 16bit accesses are always
supported
quoted
by the hardware? Does a spinlock_irqsave/spinlock_irqrestore around
the
quoted
8bit accesses already help?
Hmmm... are there any register reads that need the
same 'double cycle' sequence ??
If so you need to stop reads being interleaved (with
themselves and writes) so requesting a 16bit access
doesn't help.
Which means you need a spinlock...
David
@David: Thank You very much for that hint. You are right and to
implement correct we need a spinlock.
@Wolfgang: I was thinking about Your question regarding 8/16 bit and
in fact it wouldn't work at all on a clean 8 bit cards.
Further it wouldn't work on 16 bit cards where the MSB is not equal
to base port +1 and anyway, it's depending always on how the chip is
interfaced to the ISA bus and in which mode the chip is configured.
And therefore I was giving David's hint a try in using a spinlock in
function cc770_isa_port_write_reg_indirect() and patched as follows:
---------------------------------------------------------------------
*skb, struct net_device *dev)
stats->tx_bytes += dlc;
-
- /*
- * HM: We had some cases of repeated IRQs so make sure the
- * INT is acknowledged I know it's already further up, but
- * doing again fixed the issue
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
return NETDEV_TX_OK;
}
*dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
Please provide an extra patch for these unrelated changes. If we really
want to remove it.
Sure, this I can do.
Ok, here the patch to remove:
--------------------------------------------------------
@@ -689,12 +680,6 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o)/* Nothing more to send, switch off interrupts */cc770_write_reg(priv,msgobj[mo].ctrl0,MSGVAL_RES|TXIE_RES|RXIE_RES|INTPND_RES);-/*-*WehadsomecasesofrepeatedIRQsomakesurethe-*INTisacknowledged-*/-cc770_write_reg(priv,msgobj[mo].ctrl0,-MSGVAL_UNC|TXIE_UNC|RXIE_UNC|INTPND_RES);stats->tx_packets++;can_get_echo_skb(dev,0);----------------------------------------------------------
quoted
quoted
quoted
stats->tx_packets++;
can_get_echo_skb(dev, 0);
diff --git a/drivers/net/can/cc770/cc770_isa.c
b/drivers/net/can/cc770/cc770_isa.c
index 4be5fe2..fe39eed 100644
Do we need a global or a per device spin lock? If this should be a per
device one, please introduce a cc770_isa_priv and put the spinlock
there. Don't forget to initialize the spinlock.
Yes, that's what I was thinking as well but in the ocan driver I find:
/*
* we need a spinlock here, as the address register looks shared between
* two PC-ECAN devices. Moreover, we need to protect WRT interrupts
*/
Looks like wired hardware. Anyway, a global spinlock might be safer.
Hmmm, actually I thought to place the spinlock local because of having
the problem just with the interrupt and not with mutex.
But if global wouldn't it then better to make an array[MAX_DEV] for the
lock with initialisation in _init or _start?
But if PC-ECAN works with that configuration?
From: Wolfgang Zarre <hidden> Date: 2012-01-11 09:05:50
Hello Wolfgang,
Hello Wolfgang,
-------- Original Message --------
Subject: Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527
From: Wolfgang Grandegger <redacted>
To: info@essax.com
Cc: David Laight <redacted>, Oliver Hartkopp <socketcan@hartkopp.net>, henrik@proconx.com, netdev@vger.kernel.org, linux-can@vger.kernel.org,
socketcan-users@lists.berlios.de, IreneV [off-list ref], Stanislav Yelenskiy [off-list ref], oe@port.de, henrik@focus-sw.com
Date: Tue Jan 10 2012 17:23:59 GMT+0100 (CET)
quoted
Hi Wolfgang,
On 01/10/2012 05:13 PM, Wolfgang Zarre wrote:
quoted
Hello Wolfgang,
quoted
On 01/10/2012 01:41 PM, Wolfgang Zarre wrote:
quoted
Hello David,
quoted
quoted
cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
+ unsigned long flags;
+ spin_lock_irqsave(&outb_lock, flags);
outb(reg, base);
outb(val, base + 1);
+ spin_unlock_irqrestore(&outb_lock, flags);
Is there a 'read_reg_indirect' function??
Yes, there is.
quoted
If so it also needs to use the same mutex.
Actually, I don't think that we have a problem with mutex
beside that it's using just one inb() statement but having
for sure with an interrupt between both outb() statements which
seems to be critical for the cc770.
But the indirect read function also sets the address register before
reading the data using inb(). This sequence should also not be
interrupted and therefore we need to synchronize. For the indirect
access of the SJA1000 we also need to add spinlocks. Wonder why nobody
complained so far.
So, if I understand correct that means that inb() can be interrupted
between
setting the address and reading. If this is the case then yes, we need
spinlock if this is not the case then IMHO we wouldn't need or am I wrong?
I think we speak about different things. inb() cannot be interrupted but
outb() followed by inb(). For indirect accesses we need something like:
/* Spinlock for cc770_isa_port_write_reg_indirect */
static DEFINE_SPINLOCK(cc770_isa_port_lock);
static u8 cc770_isa_port_read_reg_indirect(const struct cc770_priv *priv,
int reg)
{
unsigned long base = (unsigned long)priv->reg_base;
unsigned long flags;
u8 val;
spin_lock_irqsave(&cc770_isa_port_lock, flags);
outb(reg, base);
val = inb(base + 1);
spin_unlock_irqrestore(&cc770_isa_port_lock, flags);
return val;
}
static void cc770_isa_port_write_reg_indirect(const struct cc770_priv *priv,
int reg, u8 val)
{
unsigned long base = (unsigned long)priv->reg_base;
unsigned long flags;
spin_lock_irqsave(&cc770_isa_port_lock, flags);
outb(reg, base);
outb(val, base + 1);
spin_unlock_irqrestore(&cc770_isa_port_lock, flags);
}
Hope we are in synch now.
Thanks a lot. Yes, now phase locked and in synch and sorry, by mistake
I looked at the wrong function (cc770_isa_port_read_reg) in the heat of
the moment.
Absolutely clear, there we need a spinlock definitely. I'll start another test run
just to confirm.
Test was successful, 2.000.000 telegrams again without problems.
Here so far the patch as You suggested:
-------------------------------------------------------------------
diff --git a/drivers/net/can/cc770/cc770_isa.c b/drivers/net/can/cc770/cc770_isa.c
index 4be5fe2..adf3e45 100644
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2012-01-11 09:31:42
On 01/11/2012 10:05 AM, Wolfgang Zarre wrote:
[...]
The patch looks good, it has some style issues though, see comments
inline. Please fix them, add a description to the patch, don't forget
your S-o-b, same applies to the other patch which removes the extra IRQ
ack. Please create a series of two patches, first this (the spinlock
patch), the second the rermoval of the extra IRQ ack. Send this patches
to this list. See [1] for more information about patch submitting.
Marc
[1] http://lxr.linux.no/linux+v3.2/Documentation/SubmittingPatches#L86
quoted hunk
Test was successful, 2.000.000 telegrams again without problems.
Here so far the patch as You suggested:
-------------------------------------------------------------------
diff --git a/drivers/net/can/cc770/cc770_isa.c
b/drivers/net/can/cc770/cc770_isa.c
index 4be5fe2..adf3e45 100644
net_device *dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
A wild guess is that this is needed to clear an interrupt
that was asserted during the ISR processing.
So you need to ack the IRQ as well as mask it.
Not sure if the difference between the xxx_UNC and xxx_RES
bits though.
David
*skb, struct net_device *dev)
stats->tx_bytes += dlc;
-
- /*
- * HM: We had some cases of repeated IRQs so make sure the
- * INT is acknowledged I know it's already further up, but
- * doing again fixed the issue
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
return NETDEV_TX_OK;
}
*dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
stats->tx_packets++;
can_get_echo_skb(dev, 0);
----------------------------------------------------------
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
net_device *dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
A wild guess is that this is needed to clear an interrupt
that was asserted during the ISR processing.
So you need to ack the IRQ as well as mask it.
I was digging a bit and as far as I understood by studying both
manuals of i82527 and cc770 the procedure is in any case most
probably not completely clean.
From my point of view there would be no need to set and reset
TXIE/RXIE after every request because according to the description
as below (e.g for TXIE):
MSB LSB Meaning
Write 0 0 Not allowed
(indeterminate)
0 1 reset
1 0 set
1 1 unchanged
TXIE Transmit Interrupt Enable
one An interrupt will be generated after a successful
transmission of a frame
zero No interrupt will be generated after a successful
transmission of a frame
The Transmit Interrupt Enable bit enables the
82527 to initiate an interrupt after the successful
transmission by the corresponding message
object This bit is written by the CPU
In fact with that we are telling the chip if we want to have
interrupts or not for a transmission and therefore it would
be enough, so far I understood, to set it at _init and switch
it of at _deinit and between just using 'unchanged'.
Actually to ack the IRQ we would have to set TXOK of the
status register to zero after reading a one for a successful
transmission but is in fact the chip doesn't care if we
reset TXOK or not.
However, if time permits I'll try to dig deeper and doing some
tests because it would need some changes more.
Anyway, I might be wrong as well because sometimes I'm checking
under heavy workload the wrong bit's as You know already ;-)
More or less the reason for the request of removal was just because
assuming that the repeated interrupts were caused as well not having
spinlock's and on the other hand if obsolete to save I/O resources
which affects mostly smaller systems with heavy load.
However, maybe Wolfgang can give an advice.
Not sure if the difference between the xxx_UNC and xxx_RES
bits though.
David
*skb, struct net_device *dev)
stats->tx_bytes += dlc;
-
- /*
- * HM: We had some cases of repeated IRQs so make sure the
- * INT is acknowledged I know it's already further up, but
- * doing again fixed the issue
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
return NETDEV_TX_OK;
}
*dev, unsigned int o)
/* Nothing more to send, switch off interrupts */
cc770_write_reg(priv, msgobj[mo].ctrl0,
MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
- /*
- * We had some cases of repeated IRQ so make sure the
- * INT is acknowledged
- */
- cc770_write_reg(priv, msgobj[mo].ctrl0,
- MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
stats->tx_packets++;
can_get_echo_skb(dev, 0);
----------------------------------------------------------
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2012-01-11 15:02:02
On 01/11/2012 03:42 PM, Wolfgang Zarre wrote:
Thanks, also for the additional hints.
You're welcome.
It might be that I let this patch for now until clarified if we
should remove or not.
Yes, the just prepare the spinlock patch.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |