login
Search
The driver was not so bad at big endian at all, only the optimised fifo
read/write functions need a fix, with this fix the driver works on
a pegasus PPC machine.
Signed-off-by: Karsten Keil <kkeil@suse.de>
---
drivers/isdn/hardware/mISDN/hfcmulti.c | 27 +++++++++++++--------------
1 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/isdn/hardware/mISDN/hfcmulti.c b/drivers/isdn/hardware/mISDN/hfcmulti.c
index 10144e8..e36360a 100644
--- a/drivers/isdn/hardware/mISDN/hfcmulti.c
+++ b/drivers/isdn/hardware/mISDN/hfcmulti.c
@@ -140,7 +140,7 @@
* #define HFC_REGISTER_DEBUG
*/
-static const char *hfcmulti_revision = "2.00";
+static const char *hfcmulti_revision = "2.01";
#include <linux/module.h>
#include <linux/pci.h>
@@ -427,12 +427,12 @@ write_fifo_regio(struct hfc_multi *hc, u_char *data, int len)
{
outb(A_FIFO_DATA0, (hc->pci_iobase)+4);
while (len>>2) {
- outl(*(u32 *)data, hc->pci_iobase);
+ outl(cpu_to_le32(*(u32 *)data), hc->pci_iobase);
data += 4;
len -= 4;
}
while (len>>1) {
- outw(*(u16 *)data, hc->pci_iobase);
+ outw(cpu_to_le16(*(u16 *)data), hc->pci_iobase);
data += 2;
len -= 2;
}
@@ -447,17 +447,19 @@ void
write_fifo_pcimem(struct hfc_multi *hc, u_char *data, int len)
{
while (len>>2) {
- writel(*(u32 *)data, (hc->pci_membase)+A_FIFO_DATA0);
+ writel(cpu_to_le32(*(u32 *)data),
+ hc->pci_membase + A_FIFO_DATA0);
data += 4;
len -= 4;
}
while (len>>1) {
- writew(*(u16 *)data, (hc->pci_membase)+A_FIFO_DATA0);
+ writew(cpu_to_le16(*(u16 *)data),
+ hc->pci_membase + A_FIFO_DATA0);
data += 2;
len -= 2;
}
while (len) {
- writeb(*data, (hc->pci_membase)+A_FIFO_DATA0);
+ writeb(*data, hc->pci_membase + A_FIFO_DATA0);
data++;
len--;
}
@@ -468,12 +470,12 @@ read_fifo_regio(struct hfc_multi *hc, u_char *data, int len)
{
outb(A_FIFO_DATA0, (hc->pci_iobase)+4);
while (len>>2) {
- *(u32 *)data = inl(hc->pci_iobase);
+ *(u32 *)data = ...Great; thanks. And the ztdummy parts? -- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation --
I on my TODO. -- Karsten Keil SuSE Labs ISDN and VOIP development SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg) --
This is however very broken... IE, you should instead use iomap and thus get ioreadXX_rep() and writeXX_rep() (XX = 16 or 32) that will do the right thing for you. IE, they will do the right amount of memory barriers and will avoid the unnecessary double-swapping you are doing there. Your code will happen to "work" but it's just way too ugly especially since you are basically re-implementing what iomap already gives you. --
Thanks for this hint, I didn't know that the repetive versions are
for byte streams and not for eg. transfer of multiple u32.
So it makes things lot easier the code should look like:
int l = len >> 2;
if (l) {
ioread32_rep(hc->pci_membase + A_FIFO_DATA0, data, l);
data += l << 2;
}
if (len & 2) {
ioread16_rep(hc->pci_membase + A_FIFO_DATA0, data, 1);
data += 2;
}
if (len & 1)
writeb(*data, hc->pci_membase + A_FIFO_DATA0);
--
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)
--
Don't mix the io* variants with the PCI variants. Use iowrite8 for the last one and make sure you do a proper pci_iomap. One cool thing with the new iomap stuff is that it also works for both PIO and MMIO, so you no longer need to differenciate writeX from outX as long as you use the right mapping stuff initially. Ben. --
Yes this stuff looks really cool, but unfortunately in our case it is not so easy to remove the different IO functions for MMIO and PIO, since for MMIO the chip use a flat register model, for PIO it use indirect addressing via only 2 ports, one for the register offset and one for the data IO. Maybe we can use the trick from lib/iomap.c to detect which kind of IO is needed, but unfortunately PIO_OFFSET, PIO_MASK and PIO_RESERVED are not exported so it would need to copy the defines, which isn't a really clean solution. -- Karsten Keil SuSE Labs ISDN and VOIP development SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg) --
Even if they were exported, you couldn't. lib/iomap.c is _not_ generic code. It's a library function for architectures that don't do it some other way. But various architectures can choose to not use lib/iomap.c at all - for example, they may have MMIO and PIO in the same address space, so they don't need the conditionals at all (because all the work was done at mapping time, not at runtime). So if you actually have different models of operation for PIO and MMIO, then yes, you need to handle that in the driver itself. Linus --
One question here, what is the better approach to do such a different
implementation, use one local function like
static void
my_out32(struct card *c, u_int offset, u-int data)
{
if (c->mode == MMIO) {
...
} else {
...
}
}
or use 2 function, one for the MMIO and one for the PIO model and then use
indirect calls (like c->my_out32(...)) ?
--
Karsten Keil
SuSE Labs
ISDN and VOIP development
SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg)
--
On Tue, 5 Aug 2008 23:02:39 +0200 Why not select PIO or MMIO at config time? Cheers, Sean --
Umm. Then you can't run a generic driver that can do either. That's the worst choice of all. As to where in the stack to do the choice - I suspect it's easier if it's done at the lowest level, but that can cause performance issues (ie testing that flag over and over again). The mmio code avoids some of the performance issues exactly by doing the "repeat" versions - so for some cases you can do the check once at the top, and then just repeat. Linus --
On Tue, 5 Aug 2008 14:37:19 -0700 (PDT) But do we really need a generic driver that can do either? Maybe for the current mISDN driver, but when you get to other chip ports, say the xhfc, you have to select the low level interface at compile time. Or I should say you currently have to select at config time. I'm not arguing that we *have* to do it this way. I just don't think we should throw out the simplest method without some thought. There is some precedence for a config time option, for example the 8139too ethernet driver. Cheers, Sean --
Which is pretty wrong thing to do. It might work fine for a specific case of an embedded vendor building one ad-hoc kernel for the device, but look at this from the point of view of a linux distribution shipping Well, yeah, we made mistakes in the past :-) If the size of the binary (or performances) involved in doing the two type of IOs in a single driver is such that having the ability to only use one is worth it (for embedded for example), then you can provide a config option that allows to select which method to build in the driver, but it's a good idea to allow building both with runtime detection. Ben. --
On Wed, 06 Aug 2008 09:04:18 +1000 Fair enough. I can agree to that. Cheers, Sean --
For cards based on the HFC mulit chips we need runtime detection, since not all card support both access methods and this is the only way how distributions can support all cards. For the xhfc this may be a differnt choice, to select the access method on compile time, because the chip is mainly used for embedded systems, so it do not need to have a generic driver, the kerne is usually configured exactly for the hardware. On the other side, if you look into the xhfc chip and documentation, it is not so different from the HFC 4/8S, so maybe it would be possible to integrate it in hfcmulti as well. And maybe here is a third way, to have a tristate CONFIG MEMIO,PIO,BOTH, which could be imlemented in the none indirect call version without overhead. I think I like this idea. -- Karsten Keil SuSE Labs ISDN and VOIP development SUSE LINUX Products GmbH, Maxfeldstr.5 90409 Nuernberg, GF: Markus Rex, HRB 16746 (AG Nuernberg) --
That's probably the best way. On powerpc, we have done a lot of work to make it possible to have kernels support multiple platforms even in the embedded space. You don't have to do it, but we found it important to allow for it. It forces to keep the code cleaner, but also makes it possible for somebody release a range of products based on different designs to support/release single binary images for the entire product range (at least provided it's the same CPU core "family", we don't currently support single binaries mixing for example 44x and 8xx processor support). What to actually do at runtime being decided based on the content of a "device-tree" passed to the kernel by the firmware or the boot wrapper. Thus, I find it a good idea to allow the option even for embedded drivers to be built with runtime detection of access method. Cheers, Ben. --
Well, it depends here if the ability to do PIO or MMIO depends on the card, then that would be stupid... Ben. --
The former is more ugly but slightly faster on some archs. Indirect function calls tend to be slightly slower than an if / else statement that can be more easily predicted by the processor. But in the case of IOs, it's not going to be a big deal, -especially- if you use the _rep forms for data transfers (and thus don't do an indirect call for each read/write). So it's totally up to you. Cheers, Ben. --
Don't use a trick like that. Not all archs use lib/iomap.c and those who don't have their own implementation using a different way of differenciating them. In any case, that's fine, you can still keep track of whether you initially mapped things IO or MEM and use different access methods without having to use different accessors. Cheers, Ben. --