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 <[email protected]>
---
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 = le32_to_cpu(inl(hc->pci_iobase));
data += 4;
len -= 4;
}
while (len>>1) {
- *(u16 *)data = inw(hc->pci_iobase);
+ *(u16 *)data = le16_to_cpu(inw(hc->pci_iobase));
data += 2;
len -= 2;
}
@@ -490,18 +492,18 @@ read_fifo_pcimem(struct hfc_multi *hc, u_char *data, int len)
{
while (len>>2) {
*(u32 *)data =
- readl((hc->pci_membase)+A_FIFO_DATA0);
+ le32_to_cpu(readl(hc->pci_membase + A_FIFO_DATA0));
data += 4;
len -= 4;
}
while (len>>1) {
*(u16 *)data =
- readw((hc->pci_membase)+A_FIFO_DATA0);
+ le16_to_cpu(readw(hc->pci_membase + A_FIFO_DATA0));
data += 2;
len -= 2;
}
while (len) {
- *data = readb((hc->pci_membase)+A_FIFO_DATA0);
+ *data = readb(hc->pci_membase + A_FIFO_DATA0);
data++;
len--;
}
@@ -5251,9 +5253,6 @@ HFCmulti_init(void)
if (debug & DEBUG_HFCMULTI_INIT)
printk(KERN_DEBUG "%s: init entered\n", __func__);
-#ifdef __BIG_ENDIAN
-#error "not running on big endian machines now"
-#endif
hfc_interrupt = symbol_get(ztdummy_extern_interrupt);
register_interrupt = symbol_get(ztdummy_register_interrupt);
unregister_interrupt = symbol_get(ztdummy_unregister_interrupt);
--
1.5.6.4
On Sat, 2008-08-02 at 16:35 +0200, Karsten Keil wrote:
> 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.
Great; thanks.
And the ztdummy parts?
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
On Mon, Aug 04, 2008 at 01:03:59PM +0100, David Woodhouse wrote:
> On Sat, 2008-08-02 at 16:35 +0200, Karsten Keil wrote:
> > 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.
>
> Great; thanks.
>
> And the ztdummy parts?
>
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)
On Sat, 2008-08-02 at 16:35 +0200, Karsten Keil wrote:
> 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.
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.
Ben.
> Signed-off-by: Karsten Keil <[email protected]>
> ---
> 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 = le32_to_cpu(inl(hc->pci_iobase));
> data += 4;
> len -= 4;
> }
> while (len>>1) {
> - *(u16 *)data = inw(hc->pci_iobase);
> + *(u16 *)data = le16_to_cpu(inw(hc->pci_iobase));
> data += 2;
> len -= 2;
> }
> @@ -490,18 +492,18 @@ read_fifo_pcimem(struct hfc_multi *hc, u_char *data, int len)
> {
> while (len>>2) {
> *(u32 *)data =
> - readl((hc->pci_membase)+A_FIFO_DATA0);
> + le32_to_cpu(readl(hc->pci_membase + A_FIFO_DATA0));
> data += 4;
> len -= 4;
> }
> while (len>>1) {
> *(u16 *)data =
> - readw((hc->pci_membase)+A_FIFO_DATA0);
> + le16_to_cpu(readw(hc->pci_membase + A_FIFO_DATA0));
> data += 2;
> len -= 2;
> }
> while (len) {
> - *data = readb((hc->pci_membase)+A_FIFO_DATA0);
> + *data = readb(hc->pci_membase + A_FIFO_DATA0);
> data++;
> len--;
> }
> @@ -5251,9 +5253,6 @@ HFCmulti_init(void)
> if (debug & DEBUG_HFCMULTI_INIT)
> printk(KERN_DEBUG "%s: init entered\n", __func__);
>
> -#ifdef __BIG_ENDIAN
> -#error "not running on big endian machines now"
> -#endif
> hfc_interrupt = symbol_get(ztdummy_extern_interrupt);
> register_interrupt = symbol_get(ztdummy_register_interrupt);
> unregister_interrupt = symbol_get(ztdummy_unregister_interrupt);
On Tue, Aug 05, 2008 at 02:29:48PM +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2008-08-02 at 16:35 +0200, Karsten Keil wrote:
> > 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.
>
> 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.
>
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)
> 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);
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.
On Tue, Aug 05, 2008 at 11:04:26PM +1000, Benjamin Herrenschmidt wrote:
>
> > 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);
>
> 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.
Yes, OK it need some cleanups in other places as well.
>
> 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.
>
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)
On Tue, 5 Aug 2008, Karsten Keil wrote:
>
> 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.
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
On Tue, Aug 05, 2008 at 11:42:56AM -0700, Linus Torvalds wrote:
>
>
> On Tue, 5 Aug 2008, Karsten Keil wrote:
> >
> > 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.
>
> 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.
>
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
"Karsten Keil" <[email protected]> wrote:
> On Tue, Aug 05, 2008 at 11:42:56AM -0700, Linus Torvalds wrote:
> >
> >
> > On Tue, 5 Aug 2008, Karsten Keil wrote:
> > >
> > > 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.
> >
> > 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.
> >
>
> 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(...)) ?
Why not select PIO or MMIO at config time?
Cheers,
Sean
On Tue, 5 Aug 2008, Sean MacLennan wrote:
>
> Why not select PIO or MMIO at config time?
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, 2008-08-05 at 19:25 +0200, Karsten Keil wrote:
> 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.
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.
> 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(...)) ?
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.
> > or use 2 function, one for the MMIO and one for the PIO model and
> > then use indirect calls (like c->my_out32(...)) ?
>
> Why not select PIO or MMIO at config time?
Well, it depends here if the ability to do PIO or MMIO depends on
the card, then that would be stupid...
Ben.
On Tue, 5 Aug 2008 14:37:19 -0700 (PDT)
"Linus Torvalds" <[email protected]> wrote:
> On Tue, 5 Aug 2008, Sean MacLennan wrote:
> >
> > Why not select PIO or MMIO at config time?
>
> Umm. Then you can't run a generic driver that can do either. That's
> the worst choice of all.
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
> 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.
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
a generic kernel image & built modules to support any HW out there...
> 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.
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
"Benjamin Herrenschmidt" <[email protected]> wrote:
> 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.
Fair enough. I can agree to that.
Cheers,
Sean
On Tue, Aug 05, 2008 at 05:59:37PM -0400, Sean MacLennan wrote:
> On Tue, 5 Aug 2008 14:37:19 -0700 (PDT)
> "Linus Torvalds" <[email protected]> wrote:
>
> > On Tue, 5 Aug 2008, Sean MacLennan wrote:
> > >
> > > Why not select PIO or MMIO at config time?
> >
> > Umm. Then you can't run a generic driver that can do either. That's
> > the worst choice of all.
>
> 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.
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)
On Wed, 2008-08-06 at 02:18 +0200, Karsten Keil wrote:
>
> 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.
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.