LinuxLists.cc - [PATCH] ARM: Gemini: Add support for PCI BUS

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Saturday 27 November 2010 13:24:35 Hans Ulli Kroll wrote:
> +#define PCI_IOSIZE_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE))
> +#define PCI_PROT_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x04)
> +#define PCI_CTRL_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x08)
> +#define PCI_SOFTRST_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x10)
> +#define PCI_CONFIG_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x28)
> +#define PCI_DATA_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x2C)

If you use the virtual address of the mapping instead of
GEMINI_PCI_IO_BASE, you don't need to repeat the IO_ADDRESS()
macro everywhere. I have a patch that gets rid of all the
conflicting definitions of this macro because it breaks
a multi-platform build once we get there.

> +static DEFINE_SPINLOCK(gemini_pci_lock);
> +
> +static struct resource gemini_pci_resource_io = {
> + .name = "PCI I/O Space",
> + .start = IO_ADDRESS(GEMINI_PCI_IO_BASE),
> + .end = IO_ADDRESS(GEMINI_PCI_IO_BASE) + SZ_1M - 1,
> + .flags = IORESOURCE_IO,
> +};
> +

This looks wrong in multiple ways:

* resources are physical addresses, not virtual addresses
* GEMINI_PCI_IO_BASE is an address in memory space, so it
needs to be IORESOURCE_MEM, not IORESOURCE_IO. You can
also register the IORESOURCE_IO resource, but that would
be .start=PCIBIOS_MIN_IO, .end=IO_SPACE_LIMIT.
* IO_SPACE_LIMIT is larger than the I/O window, which can
cause overflows. Setting it to 0xffff is generally enough.

> + spin_lock_irqsave(&gemini_pci_lock, irq_flags);
> +
> + __raw_writel(PCI_CONF_BUS(bus->number) |
> + PCI_CONF_DEVICE(PCI_SLOT(fn)) |
> + PCI_CONF_FUNCTION(PCI_FUNC(fn)) |
> + PCI_CONF_WHERE(config) |
> + PCI_CONF_ENABLE,
> + PCI_CONFIG_REG);
> +
> + switch (size) {
> + case 4:
> + __raw_writel(value, PCI_DATA_REG);
> + break;
> + case 2:
> + __raw_writew(value, PCI_DATA_REG + (config & 3));
> + break;
> + case 1:
> + __raw_writeb(value, PCI_DATA_REG + (config & 3));
> + break;
> + default:
> + ret = PCIBIOS_BAD_REGISTER_NUMBER;
> + }
> +
> + spin_unlock_irqrestore(&gemini_pci_lock, irq_flags);

The I/O ordering is probably not what you think it is.
There is no ordering guarantee between __raw_writel and
spin_lock/spin_unlock, so you really should be using
readl/writel.

Note that the pci_ops are called under another spinlock, so
you also don't need to take gemini_pci_lock here.

Arnd

2010-11-29 12:17:20

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Sun, 28 Nov 2010, Arnd Bergmann wrote:

> On Saturday 27 November 2010 13:24:35 Hans Ulli Kroll wrote:
> > +#define PCI_IOSIZE_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE))
> > +#define PCI_PROT_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x04)
> > +#define PCI_CTRL_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x08)
> > +#define PCI_SOFTRST_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x10)
> > +#define PCI_CONFIG_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x28)
> > +#define PCI_DATA_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x2C)
>
> If you use the virtual address of the mapping instead of
> GEMINI_PCI_IO_BASE, you don't need to repeat the IO_ADDRESS()
> macro everywhere. I have a patch that gets rid of all the
> conflicting definitions of this macro because it breaks
> a multi-platform build once we get there.
>
> > +static DEFINE_SPINLOCK(gemini_pci_lock);
> > +
> > +static struct resource gemini_pci_resource_io = {
> > + .name = "PCI I/O Space",
> > + .start = IO_ADDRESS(GEMINI_PCI_IO_BASE),
> > + .end = IO_ADDRESS(GEMINI_PCI_IO_BASE) + SZ_1M - 1,
> > + .flags = IORESOURCE_IO,
> > +};
> > +
>
> This looks wrong in multiple ways:
>
> * resources are physical addresses, not virtual addresses
> * GEMINI_PCI_IO_BASE is an address in memory space, so it
> needs to be IORESOURCE_MEM, not IORESOURCE_IO. You can
> also register the IORESOURCE_IO resource, but that would
> be .start=PCIBIOS_MIN_IO, .end=IO_SPACE_LIMIT.
> * IO_SPACE_LIMIT is larger than the I/O window, which can
> cause overflows. Setting it to 0xffff is generally enough.
>

So I must remove these lines ??
}, {
.virtual = IO_ADDRESS(GEMINI_PCI_IO_BASE),
.pfn = __phys_to_pfn(GEMINI_PCI_IO_BASE),
.length = SZ_512K,
.type = MT_DEVICE,
},
These are from arch/arm/mach-gemini/mm.c

> > + spin_lock_irqsave(&gemini_pci_lock, irq_flags);
> > +
> > + __raw_writel(PCI_CONF_BUS(bus->number) |
> > + PCI_CONF_DEVICE(PCI_SLOT(fn)) |
> > + PCI_CONF_FUNCTION(PCI_FUNC(fn)) |
> > + PCI_CONF_WHERE(config) |
> > + PCI_CONF_ENABLE,
> > + PCI_CONFIG_REG);
> > +
> > + switch (size) {
> > + case 4:
> > + __raw_writel(value, PCI_DATA_REG);
> > + break;
> > + case 2:
> > + __raw_writew(value, PCI_DATA_REG + (config & 3));
> > + break;
> > + case 1:
> > + __raw_writeb(value, PCI_DATA_REG + (config & 3));
> > + break;
> > + default:
> > + ret = PCIBIOS_BAD_REGISTER_NUMBER;
> > + }
> > +
> > + spin_unlock_irqrestore(&gemini_pci_lock, irq_flags);
>
> The I/O ordering is probably not what you think it is.
> There is no ordering guarantee between __raw_writel and
> spin_lock/spin_unlock, so you really should be using
> readl/writel.
>
> Note that the pci_ops are called under another spinlock, so
> you also don't need to take gemini_pci_lock here.
>
> Arnd
>

2010-11-29 15:02:58

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Monday 29 November 2010, Hans Ulli Kroll wrote:
> >
> > This looks wrong in multiple ways:
> >
> > * resources are physical addresses, not virtual addresses
> > * GEMINI_PCI_IO_BASE is an address in memory space, so it
> > needs to be IORESOURCE_MEM, not IORESOURCE_IO. You can
> > also register the IORESOURCE_IO resource, but that would
> > be .start=PCIBIOS_MIN_IO, .end=IO_SPACE_LIMIT.
> > * IO_SPACE_LIMIT is larger than the I/O window, which can
> > cause overflows. Setting it to 0xffff is generally enough.
> >
>
> So I must remove these lines ??
> }, {
> .virtual = IO_ADDRESS(GEMINI_PCI_IO_BASE),
> .pfn = __phys_to_pfn(GEMINI_PCI_IO_BASE),
> .length = SZ_512K,
> .type = MT_DEVICE,
> },
> These are from arch/arm/mach-gemini/mm.c

No, these look right, just fix the issues I mentioned, i.e.
the definition of IO_SPACE_LIMIT and the resource registration.

Arnd

2010-11-29 16:11:23

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On 11/28/2010 09:56 PM, Arnd Bergmann wrote:
> On Saturday 27 November 2010 13:24:35 Hans Ulli Kroll wrote:
>> +#define PCI_IOSIZE_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE))
>> +#define PCI_PROT_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x04)
>> +#define PCI_CTRL_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x08)
>> +#define PCI_SOFTRST_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x10)
>> +#define PCI_CONFIG_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x28)
>> +#define PCI_DATA_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x2C)
>
> If you use the virtual address of the mapping instead of
> GEMINI_PCI_IO_BASE, you don't need to repeat the IO_ADDRESS()
> macro everywhere. I have a patch that gets rid of all the
> conflicting definitions of this macro because it breaks
> a multi-platform build once we get there.
>
>> +static DEFINE_SPINLOCK(gemini_pci_lock);
>> +
>> +static struct resource gemini_pci_resource_io = {
>> + .name = "PCI I/O Space",
>> + .start = IO_ADDRESS(GEMINI_PCI_IO_BASE),
>> + .end = IO_ADDRESS(GEMINI_PCI_IO_BASE) + SZ_1M - 1,
>> + .flags = IORESOURCE_IO,
>> +};
>> +
>
> This looks wrong in multiple ways:
>
> * resources are physical addresses, not virtual addresses
> * GEMINI_PCI_IO_BASE is an address in memory space, so it
> needs to be IORESOURCE_MEM, not IORESOURCE_IO. You can
> also register the IORESOURCE_IO resource, but that would
> be .start=PCIBIOS_MIN_IO, .end=IO_SPACE_LIMIT.
> * IO_SPACE_LIMIT is larger than the I/O window, which can
> cause overflows. Setting it to 0xffff is generally enough.
>
>> + spin_lock_irqsave(&gemini_pci_lock, irq_flags);
>> +
>> + __raw_writel(PCI_CONF_BUS(bus->number) |
>> + PCI_CONF_DEVICE(PCI_SLOT(fn)) |
>> + PCI_CONF_FUNCTION(PCI_FUNC(fn)) |
>> + PCI_CONF_WHERE(config) |
>> + PCI_CONF_ENABLE,
>> + PCI_CONFIG_REG);
>> +
>> + switch (size) {
>> + case 4:
>> + __raw_writel(value, PCI_DATA_REG);
>> + break;
>> + case 2:
>> + __raw_writew(value, PCI_DATA_REG + (config& 3));
>> + break;
>> + case 1:
>> + __raw_writeb(value, PCI_DATA_REG + (config& 3));
>> + break;
>> + default:
>> + ret = PCIBIOS_BAD_REGISTER_NUMBER;
>> + }
>> +
>> + spin_unlock_irqrestore(&gemini_pci_lock, irq_flags);
>
> The I/O ordering is probably not what you think it is.
> There is no ordering guarantee between __raw_writel and
> spin_lock/spin_unlock, so you really should be using
> readl/writel.

No he really should NOT use readl/writel. The ONLY difference
between readl/writel and __raw_readl/__raw_writel is endianess
conversion. __raw_*l is not doing it. Which to use depend only
on HW.

> Note that the pci_ops are called under another spinlock, so
> you also don't need to take gemini_pci_lock here.
>
> Arnd

2010-11-29 16:45:31

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Monday 29 November 2010, Paulius Zaleckas wrote:
> > The I/O ordering is probably not what you think it is.
> > There is no ordering guarantee between __raw_writel and
> > spin_lock/spin_unlock, so you really should be using
> > readl/writel.
>
> No he really should NOT use readl/writel. The ONLY difference
> between readl/writel and __raw_readl/__raw_writel is endianess
> conversion. __raw_*l is not doing it. Which to use depend only
> on HW.

There are many differences between readl and __raw_readl, including

* __raw_readl does not have barriers and does not serialize with
spinlocks, so it breaks on out-of-order CPUs.
* __raw_readl does not have a specific endianess, while readl is
fixed little-endian, just as the hardware is in this case.
The endian-conversion is a NOP on little-endian ARM, but required
if you actually run on a big-endian ARM (you don't).
* __raw_readl may not be atomic, gcc is free to split the access
into byte wise reads (it normally does not, unless you mark
the pointer __attribute__((packed))).

In essence, it is almost never a good idea to use __raw_readl, and
the double underscores should tell you so.

Arnd

2010-11-29 18:52:59

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On 11/29/2010 06:45 PM, Arnd Bergmann wrote:
> On Monday 29 November 2010, Paulius Zaleckas wrote:
>>> The I/O ordering is probably not what you think it is.
>>> There is no ordering guarantee between __raw_writel and
>>> spin_lock/spin_unlock, so you really should be using
>>> readl/writel.
>>
>> No he really should NOT use readl/writel. The ONLY difference
>> between readl/writel and __raw_readl/__raw_writel is endianess
>> conversion. __raw_*l is not doing it. Which to use depend only
>> on HW.
>
> There are many differences between readl and __raw_readl, including
>
> * __raw_readl does not have barriers and does not serialize with
> spinlocks, so it breaks on out-of-order CPUs.
> * __raw_readl does not have a specific endianess, while readl is
> fixed little-endian, just as the hardware is in this case.
> The endian-conversion is a NOP on little-endian ARM, but required
> if you actually run on a big-endian ARM (you don't).
> * __raw_readl may not be atomic, gcc is free to split the access
> into byte wise reads (it normally does not, unless you mark
> the pointer __attribute__((packed))).
>
> In essence, it is almost never a good idea to use __raw_readl, and
> the double underscores should tell you so.

You are wrong:

Since CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined for FA526 core,
no barriers are in use when using readl. It just translates into
le32_to_cpu(__raw_readl(x)). Now this CPU has physical pin for endianess
configuration and if you will chose big-endian you will fail to read
internal registers, because they ALSO change endianess and le32_to_cpu()
will screw it. However it is different when accessing registers through
PCI bus, then you need to use readl().

2010-11-29 19:33:08

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Mon, Nov 29, 2010 at 06:05:07PM +0200, Paulius Zaleckas wrote:
> No he really should NOT use readl/writel. The ONLY difference
> between readl/writel and __raw_readl/__raw_writel is endianess
> conversion. __raw_*l is not doing it. Which to use depend only
> on HW.

Wrong. readl/writel have barriers too to guarantee ordering between
device accesses and memory accesses. (device accesses are already
ordered with respect to themselves.) __raw variants do not
guarantee the relative ordering between memory accesses and device
accesses.

2010-11-29 19:57:28

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On 11/29/2010 09:32 PM, Russell King - ARM Linux wrote:
> On Mon, Nov 29, 2010 at 06:05:07PM +0200, Paulius Zaleckas wrote:
>> No he really should NOT use readl/writel. The ONLY difference
>> between readl/writel and __raw_readl/__raw_writel is endianess
>> conversion. __raw_*l is not doing it. Which to use depend only
>> on HW.
>
> Wrong. readl/writel have barriers too to guarantee ordering between
> device accesses and memory accesses. (device accesses are already
> ordered with respect to themselves.) __raw variants do not
> guarantee the relative ordering between memory accesses and device
> accesses.

#ifdef CONFIG_ARM_DMA_MEM_BUFFERABLE
#define __iormb() rmb()
#define __iowmb() wmb()
#else
#define __iormb() do { } while (0)
#define __iowmb() do { } while (0)
#endif

CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined in this case.
I don't see any other barriers.
readl does endianess conversion. What to do if you don't need it?
Do it one more time?

2010-11-29 20:03:05

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Monday 29 November 2010 19:52:55 Paulius Zaleckas wrote:
> On 11/29/2010 06:45 PM, Arnd Bergmann wrote:
> > There are many differences between readl and __raw_readl, including
> >
> > * __raw_readl does not have barriers and does not serialize with
> > spinlocks, so it breaks on out-of-order CPUs.
> > * __raw_readl does not have a specific endianess, while readl is
> > fixed little-endian, just as the hardware is in this case.
> > The endian-conversion is a NOP on little-endian ARM, but required
> > if you actually run on a big-endian ARM (you don't).
> > * __raw_readl may not be atomic, gcc is free to split the access
> > into byte wise reads (it normally does not, unless you mark
> > the pointer __attribute__((packed))).
> >
> > In essence, it is almost never a good idea to use __raw_readl, and
> > the double underscores should tell you so.
>
> You are wrong:
>
> Since CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined for FA526 core,
> no barriers are in use when using readl. It just translates into
> le32_to_cpu(__raw_readl(x)). Now this CPU has physical pin for endianess
> configuration and if you will chose big-endian you will fail to read
> internal registers, because they ALSO change endianess and le32_to_cpu()
> will screw it. However it is different when accessing registers through
> PCI bus, then you need to use readl().

Ok, I only checked that the platform does not support big-endian Linux
kernel, not if the HW designers screwed up their registers, sorry about
that.

The other points are of course still valid: If the code ever gets
used on an out of order CPU, it is broken. More importantly, if someone
looks at the code as an example for writing another PCI support code,
it may end up getting copied to some place where it ends up causing
trouble.

The typical way to deal with mixed-endian hardware reliably is to have
a header file containing code like

#ifdef CONFIG_GEMINI_BIG_ENDIAN_IO
#define gemini_readl(x) __swab32(readl(x))
#define ...
#else
#define gemini_readl(x) readl(x))
#endif

This also takes care of the (not as unlikely as you'd hope) case that
the next person reusing the PCI hardware wires its endianess different
from the CPU endianess.

Arnd

2010-11-29 20:20:06

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On 11/29/2010 10:02 PM, Arnd Bergmann wrote:
> On Monday 29 November 2010 19:52:55 Paulius Zaleckas wrote:
>> On 11/29/2010 06:45 PM, Arnd Bergmann wrote:
>>> There are many differences between readl and __raw_readl, including
>>>
>>> * __raw_readl does not have barriers and does not serialize with
>>> spinlocks, so it breaks on out-of-order CPUs.
>>> * __raw_readl does not have a specific endianess, while readl is
>>> fixed little-endian, just as the hardware is in this case.
>>> The endian-conversion is a NOP on little-endian ARM, but required
>>> if you actually run on a big-endian ARM (you don't).
>>> * __raw_readl may not be atomic, gcc is free to split the access
>>> into byte wise reads (it normally does not, unless you mark
>>> the pointer __attribute__((packed))).
>>>
>>> In essence, it is almost never a good idea to use __raw_readl, and
>>> the double underscores should tell you so.
>>
>> You are wrong:
>>
>> Since CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined for FA526 core,
>> no barriers are in use when using readl. It just translates into
>> le32_to_cpu(__raw_readl(x)). Now this CPU has physical pin for endianess
>> configuration and if you will chose big-endian you will fail to read
>> internal registers, because they ALSO change endianess and le32_to_cpu()
>> will screw it. However it is different when accessing registers through
>> PCI bus, then you need to use readl().
>
> Ok, I only checked that the platform does not support big-endian Linux
> kernel, not if the HW designers screwed up their registers, sorry about
> that.
>
> The other points are of course still valid: If the code ever gets
> used on an out of order CPU, it is broken. More importantly, if someone
> looks at the code as an example for writing another PCI support code,
> it may end up getting copied to some place where it ends up causing
> trouble.
>
> The typical way to deal with mixed-endian hardware reliably is to have
> a header file containing code like
>
> #ifdef CONFIG_GEMINI_BIG_ENDIAN_IO
> #define gemini_readl(x) __swab32(readl(x))
> #define ...
> #else
> #define gemini_readl(x) readl(x))
> #endif
>
> This also takes care of the (not as unlikely as you'd hope) case that
> the next person reusing the PCI hardware wires its endianess different
> from the CPU endianess.

Actually I am not very sure how CPU works in big endian mode :)
I have never tried it and I think only some guys who made it did that.
So readl will work for 99.99% of cases. In datasheet they say that:
"All registers in Gemini use Little Endian and must be accessed by aligned
32-bit word operations. The bus connection interface logic provides an Endian
Conversion function."
For me it looks like it can mean whatever you want :)

2010-11-30 08:15:19

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Mon, 29 Nov 2010, Paulius Zaleckas wrote:

> On 11/29/2010 10:02 PM, Arnd Bergmann wrote:
> > On Monday 29 November 2010 19:52:55 Paulius Zaleckas wrote:
> > > On 11/29/2010 06:45 PM, Arnd Bergmann wrote:
> > > > There are many differences between readl and __raw_readl, including
> > > >
> > > > * __raw_readl does not have barriers and does not serialize with
> > > > spinlocks, so it breaks on out-of-order CPUs.
> > > > * __raw_readl does not have a specific endianess, while readl is
> > > > fixed little-endian, just as the hardware is in this case.
> > > > The endian-conversion is a NOP on little-endian ARM, but required
> > > > if you actually run on a big-endian ARM (you don't).
> > > > * __raw_readl may not be atomic, gcc is free to split the access
> > > > into byte wise reads (it normally does not, unless you mark
> > > > the pointer __attribute__((packed))).
> > > >
> > > > In essence, it is almost never a good idea to use __raw_readl, and
> > > > the double underscores should tell you so.
> > >
> > > You are wrong:
> > >
> > > Since CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined for FA526 core,
> > > no barriers are in use when using readl. It just translates into
> > > le32_to_cpu(__raw_readl(x)). Now this CPU has physical pin for endianess
> > > configuration and if you will chose big-endian you will fail to read
> > > internal registers, because they ALSO change endianess and le32_to_cpu()
> > > will screw it. However it is different when accessing registers through
> > > PCI bus, then you need to use readl().
> >
> > Ok, I only checked that the platform does not support big-endian Linux
> > kernel, not if the HW designers screwed up their registers, sorry about
> > that.
> >
> > The other points are of course still valid: If the code ever gets
> > used on an out of order CPU, it is broken. More importantly, if someone
> > looks at the code as an example for writing another PCI support code,
> > it may end up getting copied to some place where it ends up causing
> > trouble.
> >
> > The typical way to deal with mixed-endian hardware reliably is to have
> > a header file containing code like
> >
> > #ifdef CONFIG_GEMINI_BIG_ENDIAN_IO
> > #define gemini_readl(x) __swab32(readl(x))
> > #define ...
> > #else
> > #define gemini_readl(x) readl(x))
> > #endif
> >
> > This also takes care of the (not as unlikely as you'd hope) case that
> > the next person reusing the PCI hardware wires its endianess different
> > from the CPU endianess.
>
> Actually I am not very sure how CPU works in big endian mode :)
> I have never tried it and I think only some guys who made it did that.
> So readl will work for 99.99% of cases. In datasheet they say that:
> "All registers in Gemini use Little Endian and must be accessed by aligned
> 32-bit word operations. The bus connection interface logic provides an Endian
> Conversion function."
> For me it looks like it can mean whatever you want :)
>

I think the endianes pin switched only the cpu, not the hardware
registers.

Here is some sample code from the ethernet devive on Gemini
typedef union
{
unsigned int bits32;
struct bit
{
#if (BIG_ENDIAN==1)
unsigned int reserved : 15; // bit 31:17
unsigned int v_bit_mode : 1; // bit 16
unsigned int device_id : 12; // bit 15:4
unsigned int revision_id : 4; // bit 3:0
#else
unsigned int revision_id : 4; // bit 3:0
unsigned int device_id : 12; // bit 15:4
unsigned int v_bit_mode : 1; // bit 16
unsigned int reserved : 15; // bit 31:17
#endif
} bits;
} TOE_VERSION_T;

2010-11-30 09:34:59

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Tue, Nov 30, 2010 at 10:15 AM, Hans Ulli Kroll
<[email protected]> wrote:
>
>
> On Mon, 29 Nov 2010, Paulius Zaleckas wrote:
>
>> On 11/29/2010 10:02 PM, Arnd Bergmann wrote:
>> > On Monday 29 November 2010 19:52:55 Paulius Zaleckas wrote:
>> > > On 11/29/2010 06:45 PM, Arnd Bergmann wrote:
>> > > > There are many differences between readl and __raw_readl, including
>> > > >
>> > > > * __raw_readl does not have barriers and does not serialize with
>> > > > ? ? spinlocks, so it breaks on out-of-order CPUs.
>> > > > * __raw_readl does not have a specific endianess, while readl is
>> > > > ? ? fixed little-endian, just as the hardware is in this case.
>> > > > ? ? The endian-conversion is a NOP on little-endian ARM, but required
>> > > > ? ? if you actually run on a big-endian ARM (you don't).
>> > > > * __raw_readl may not be atomic, gcc is free to split the access
>> > > > ? ? into byte wise reads (it normally does not, unless you mark
>> > > > ? ? the pointer __attribute__((packed))).
>> > > >
>> > > > In essence, it is almost never a good idea to use __raw_readl, and
>> > > > the double underscores should tell you so.
>> > >
>> > > You are wrong:
>> > >
>> > > Since CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined for FA526 core,
>> > > no barriers are in use when using readl. It just translates into
>> > > le32_to_cpu(__raw_readl(x)). Now this CPU has physical pin for endianess
>> > > configuration and if you will chose big-endian you will fail to read
>> > > internal registers, because they ALSO change endianess and le32_to_cpu()
>> > > will screw it. However it is different when accessing registers through
>> > > PCI bus, then you need to use readl().
>> >
>> > Ok, I only checked that the platform does not support big-endian Linux
>> > kernel, not if the HW designers screwed up their registers, sorry about
>> > that.
>> >
>> > The other points are of course still valid: If the code ever gets
>> > used on an out of order CPU, it is broken. More importantly, if someone
>> > looks at the code as an example for writing another PCI support code,
>> > it may end up getting copied to some place where it ends up causing
>> > trouble.
>> >
>> > The typical way to deal with mixed-endian hardware reliably is to have
>> > a header file containing code like
>> >
>> > #ifdef CONFIG_GEMINI_BIG_ENDIAN_IO
>> > #define gemini_readl(x) __swab32(readl(x))
>> > #define ...
>> > #else
>> > #define gemini_readl(x) readl(x))
>> > #endif
>> >
>> > This also takes care of the (not as unlikely as you'd hope) case that
>> > the next person reusing the PCI hardware wires its endianess different
>> > from the CPU endianess.
>>
>> Actually I am not very sure how CPU works in big endian mode :)
>> I have never tried it and I think only some guys who made it did that.
>> So readl will work for 99.99% of cases. In datasheet they say that:
>> "All registers in Gemini use Little Endian and must be accessed by aligned
>> 32-bit word operations. The bus connection interface logic provides an Endian
>> Conversion function."
>> For me it looks like it can mean whatever you want :)
>>
>
> I think the endianes pin switched only the cpu, not the hardware
> registers.

Yes, but original driver used readl/writel and it does le32_to_cpu,
so that structure definition is just reversing it.
If you will use __raw_readl/__raw_writel than there will be no need
for this redefinition.

> Here is some sample code from the ethernet devive on Gemini
> typedef union
> {
> ? ? ? ?unsigned int bits32;
> ? ? ? ?struct bit
> ? ? ? ?{
> #if (BIG_ENDIAN==1)
> ? ? ? ? ? ? ? ?unsigned int reserved ? ? ? ? ? : 15; ? // bit 31:17
> ? ? ? ? ? ? ? ?unsigned int v_bit_mode ? ? ? ? : 1; ? ?// bit 16
> ? ? ? ? ? ? ? ?unsigned int device_id ? ? ? ? ?: 12; ? // bit 15:4
> ? ? ? ? ? ? ? ?unsigned int revision_id ? ? ? ?: 4; ? ?// bit ?3:0
> #else
> ? ? ? ? ? ? ? ?unsigned int revision_id ? ? ? ?: 4; ? ?// bit ?3:0
> ? ? ? ? ? ? ? ?unsigned int device_id ? ? ? ? ?: 12; ? // bit 15:4
> ? ? ? ? ? ? ? ?unsigned int v_bit_mode ? ? ? ? : 1; ? ?// bit 16
> ? ? ? ? ? ? ? ?unsigned int reserved ? ? ? ? ? : 15; ? // bit 31:17
> #endif

The other thing is that this endianess redefinition is very starnge since
it should swap bytes and not bits inside this struct. So I assume that
big endian was never tested on this driver and it will not work.
But ofcouse I can be wrong here :)

> ? ? ? ?} bits;
> } TOE_VERSION_T;
>
>
>

2010-12-01 11:52:49

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Tue, 30 Nov 2010, Paulius Zaleckas wrote:

> On Tue, Nov 30, 2010 at 10:15 AM, Hans Ulli Kroll
> <[email protected]> wrote:
> >
> >
> > On Mon, 29 Nov 2010, Paulius Zaleckas wrote:
> >
> >> On 11/29/2010 10:02 PM, Arnd Bergmann wrote:
> >> > On Monday 29 November 2010 19:52:55 Paulius Zaleckas wrote:
> >> > > On 11/29/2010 06:45 PM, Arnd Bergmann wrote:
> >> > > > There are many differences between readl and __raw_readl, including
> >> > > >
> >> > > > * __raw_readl does not have barriers and does not serialize with
> >> > > > ? ? spinlocks, so it breaks on out-of-order CPUs.
> >> > > > * __raw_readl does not have a specific endianess, while readl is
> >> > > > ? ? fixed little-endian, just as the hardware is in this case.
> >> > > > ? ? The endian-conversion is a NOP on little-endian ARM, but required
> >> > > > ? ? if you actually run on a big-endian ARM (you don't).
> >> > > > * __raw_readl may not be atomic, gcc is free to split the access
> >> > > > ? ? into byte wise reads (it normally does not, unless you mark
> >> > > > ? ? the pointer __attribute__((packed))).
> >> > > >
> >> > > > In essence, it is almost never a good idea to use __raw_readl, and
> >> > > > the double underscores should tell you so.
> >> > >
> >> > > You are wrong:
> >> > >
> >> > > Since CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined for FA526 core,
> >> > > no barriers are in use when using readl. It just translates into
> >> > > le32_to_cpu(__raw_readl(x)). Now this CPU has physical pin for endianess
> >> > > configuration and if you will chose big-endian you will fail to read
> >> > > internal registers, because they ALSO change endianess and le32_to_cpu()
> >> > > will screw it. However it is different when accessing registers through
> >> > > PCI bus, then you need to use readl().
> >> >
> >> > Ok, I only checked that the platform does not support big-endian Linux
> >> > kernel, not if the HW designers screwed up their registers, sorry about
> >> > that.
> >> >
> >> > The other points are of course still valid: If the code ever gets
> >> > used on an out of order CPU, it is broken. More importantly, if someone
> >> > looks at the code as an example for writing another PCI support code,
> >> > it may end up getting copied to some place where it ends up causing
> >> > trouble.
> >> >
> >> > The typical way to deal with mixed-endian hardware reliably is to have
> >> > a header file containing code like
> >> >
> >> > #ifdef CONFIG_GEMINI_BIG_ENDIAN_IO
> >> > #define gemini_readl(x) __swab32(readl(x))
> >> > #define ...
> >> > #else
> >> > #define gemini_readl(x) readl(x))
> >> > #endif
> >> >
> >> > This also takes care of the (not as unlikely as you'd hope) case that
> >> > the next person reusing the PCI hardware wires its endianess different
> >> > from the CPU endianess.
> >>
> >> Actually I am not very sure how CPU works in big endian mode :)
> >> I have never tried it and I think only some guys who made it did that.
> >> So readl will work for 99.99% of cases. In datasheet they say that:
> >> "All registers in Gemini use Little Endian and must be accessed by aligned
> >> 32-bit word operations. The bus connection interface logic provides an Endian
> >> Conversion function."
> >> For me it looks like it can mean whatever you want :)
> >>
> >
> > I think the endianes pin switched only the cpu, not the hardware
> > registers.
>
> Yes, but original driver used readl/writel and it does le32_to_cpu,
> so that structure definition is just reversing it.
> If you will use __raw_readl/__raw_writel than there will be no need
> for this redefinition.
>
> > Here is some sample code from the ethernet devive on Gemini
> > typedef union
> > {
> > ? ? ? ?unsigned int bits32;
> > ? ? ? ?struct bit
> > ? ? ? ?{
> > #if (BIG_ENDIAN==1)
> > ? ? ? ? ? ? ? ?unsigned int reserved ? ? ? ? ? : 15; ? // bit 31:17
> > ? ? ? ? ? ? ? ?unsigned int v_bit_mode ? ? ? ? : 1; ? ?// bit 16
> > ? ? ? ? ? ? ? ?unsigned int device_id ? ? ? ? ?: 12; ? // bit 15:4
> > ? ? ? ? ? ? ? ?unsigned int revision_id ? ? ? ?: 4; ? ?// bit ?3:0
> > #else
> > ? ? ? ? ? ? ? ?unsigned int revision_id ? ? ? ?: 4; ? ?// bit ?3:0
> > ? ? ? ? ? ? ? ?unsigned int device_id ? ? ? ? ?: 12; ? // bit 15:4
> > ? ? ? ? ? ? ? ?unsigned int v_bit_mode ? ? ? ? : 1; ? ?// bit 16
> > ? ? ? ? ? ? ? ?unsigned int reserved ? ? ? ? ? : 15; ? // bit 31:17
> > #endif
>
> The other thing is that this endianess redefinition is very starnge since
> it should swap bytes and not bits inside this struct. So I assume that
> big endian was never tested on this driver and it will not work.
> But ofcouse I can be wrong here :)
>

At this momment my brain restarts in very slow motion mode ;-)
This can't work. The definition Storlinksemi uses for swapping bits and
bytes are totaly wrong.
They never _even_ testet this, or understand little endian or big endian.
Take this simple sample

typedef union {
unsigned int bits32;
struct bit {
#if (BIG_ENDIAN==0)
unsigned int a : 1;
unsigned int b : 31;
#else
unsigned int b : 31;
unsigned int a : 1;
#endif
};
} TEST;

They swaped the bits inside one byte

> > ? ? ? ?} bits;
> > } TOE_VERSION_T;
> >
> >
> >
>

2010-12-01 13:08:09

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Wed, Dec 1, 2010 at 1:52 PM, Hans Ulli Kroll
<[email protected]> wrote:
>
>
> On Tue, 30 Nov 2010, Paulius Zaleckas wrote:
>
>> On Tue, Nov 30, 2010 at 10:15 AM, Hans Ulli Kroll
>> <[email protected]> wrote:
>> >
>> >
>> > On Mon, 29 Nov 2010, Paulius Zaleckas wrote:
>> >
>> >> On 11/29/2010 10:02 PM, Arnd Bergmann wrote:
>> >> > On Monday 29 November 2010 19:52:55 Paulius Zaleckas wrote:
>> >> > > On 11/29/2010 06:45 PM, Arnd Bergmann wrote:
>> >> > > > There are many differences between readl and __raw_readl, including
>> >> > > >
>> >> > > > * __raw_readl does not have barriers and does not serialize with
>> >> > > > ? ? spinlocks, so it breaks on out-of-order CPUs.
>> >> > > > * __raw_readl does not have a specific endianess, while readl is
>> >> > > > ? ? fixed little-endian, just as the hardware is in this case.
>> >> > > > ? ? The endian-conversion is a NOP on little-endian ARM, but required
>> >> > > > ? ? if you actually run on a big-endian ARM (you don't).
>> >> > > > * __raw_readl may not be atomic, gcc is free to split the access
>> >> > > > ? ? into byte wise reads (it normally does not, unless you mark
>> >> > > > ? ? the pointer __attribute__((packed))).
>> >> > > >
>> >> > > > In essence, it is almost never a good idea to use __raw_readl, and
>> >> > > > the double underscores should tell you so.
>> >> > >
>> >> > > You are wrong:
>> >> > >
>> >> > > Since CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined for FA526 core,
>> >> > > no barriers are in use when using readl. It just translates into
>> >> > > le32_to_cpu(__raw_readl(x)). Now this CPU has physical pin for endianess
>> >> > > configuration and if you will chose big-endian you will fail to read
>> >> > > internal registers, because they ALSO change endianess and le32_to_cpu()
>> >> > > will screw it. However it is different when accessing registers through
>> >> > > PCI bus, then you need to use readl().
>> >> >
>> >> > Ok, I only checked that the platform does not support big-endian Linux
>> >> > kernel, not if the HW designers screwed up their registers, sorry about
>> >> > that.
>> >> >
>> >> > The other points are of course still valid: If the code ever gets
>> >> > used on an out of order CPU, it is broken. More importantly, if someone
>> >> > looks at the code as an example for writing another PCI support code,
>> >> > it may end up getting copied to some place where it ends up causing
>> >> > trouble.
>> >> >
>> >> > The typical way to deal with mixed-endian hardware reliably is to have
>> >> > a header file containing code like
>> >> >
>> >> > #ifdef CONFIG_GEMINI_BIG_ENDIAN_IO
>> >> > #define gemini_readl(x) __swab32(readl(x))
>> >> > #define ...
>> >> > #else
>> >> > #define gemini_readl(x) readl(x))
>> >> > #endif
>> >> >
>> >> > This also takes care of the (not as unlikely as you'd hope) case that
>> >> > the next person reusing the PCI hardware wires its endianess different
>> >> > from the CPU endianess.
>> >>
>> >> Actually I am not very sure how CPU works in big endian mode :)
>> >> I have never tried it and I think only some guys who made it did that.
>> >> So readl will work for 99.99% of cases. In datasheet they say that:
>> >> "All registers in Gemini use Little Endian and must be accessed by aligned
>> >> 32-bit word operations. The bus connection interface logic provides an Endian
>> >> Conversion function."
>> >> For me it looks like it can mean whatever you want :)
>> >>
>> >
>> > I think the endianes pin switched only the cpu, not the hardware
>> > registers.
>>
>> Yes, but original driver used readl/writel and it does le32_to_cpu,
>> so that structure definition is just reversing it.
>> If you will use __raw_readl/__raw_writel than there will be no need
>> for this redefinition.
>>
>> > Here is some sample code from the ethernet devive on Gemini
>> > typedef union
>> > {
>> > ? ? ? ?unsigned int bits32;
>> > ? ? ? ?struct bit
>> > ? ? ? ?{
>> > #if (BIG_ENDIAN==1)
>> > ? ? ? ? ? ? ? ?unsigned int reserved ? ? ? ? ? : 15; ? // bit 31:17
>> > ? ? ? ? ? ? ? ?unsigned int v_bit_mode ? ? ? ? : 1; ? ?// bit 16
>> > ? ? ? ? ? ? ? ?unsigned int device_id ? ? ? ? ?: 12; ? // bit 15:4
>> > ? ? ? ? ? ? ? ?unsigned int revision_id ? ? ? ?: 4; ? ?// bit ?3:0
>> > #else
>> > ? ? ? ? ? ? ? ?unsigned int revision_id ? ? ? ?: 4; ? ?// bit ?3:0
>> > ? ? ? ? ? ? ? ?unsigned int device_id ? ? ? ? ?: 12; ? // bit 15:4
>> > ? ? ? ? ? ? ? ?unsigned int v_bit_mode ? ? ? ? : 1; ? ?// bit 16
>> > ? ? ? ? ? ? ? ?unsigned int reserved ? ? ? ? ? : 15; ? // bit 31:17
>> > #endif
>>
>> The other thing is that this endianess redefinition is very starnge since
>> it should swap bytes and not bits inside this struct. So I assume that
>> big endian was never tested on this driver and it will not work.
>> But ofcouse I can be wrong here :)
>>
>
> At this momment my brain restarts in very slow motion mode ;-)
> This can't work. The definition Storlinksemi uses for swapping bits and
> bytes are totaly wrong.
> They never _even_ testet this, or understand little endian or big endian.
> Take this simple sample
>
> typedef union {
> ? ? ? ?unsigned int bits32;
> ? ? ? ?struct bit {
> #if (BIG_ENDIAN==0)
> ? ? ? ? ? ? ? ?unsigned int a ?: 1;
> ? ? ? ? ? ? ? ?unsigned int b ?: 31;
> #else
> ? ? ? ? ? ? ? ?unsigned int b ?: 31;
> ? ? ? ? ? ? ? ?unsigned int a ?: 1;
> #endif
> ? ? ? ?};
> } TEST;
>
> They swaped the bits inside one byte

Ha! Wait a minute. Looks like we are both wrong...
Read the beginning of: http://thread.gmane.org/gmane.linux.kernel/1037608

So it means we should use readl/writel and get rid of these non-portable
bit-fields...

>> > ? ? ? ?} bits;
>> > } TOE_VERSION_T;
>> >
>> >
>> >
>>

2010-12-01 15:02:39

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

On Wed, 1 Dec 2010, Paulius Zaleckas wrote:

> On Wed, Dec 1, 2010 at 1:52 PM, Hans Ulli Kroll
> <[email protected]> wrote:
> >
> >
> > On Tue, 30 Nov 2010, Paulius Zaleckas wrote:
> >
> >> On Tue, Nov 30, 2010 at 10:15 AM, Hans Ulli Kroll
> >> <[email protected]> wrote:
> >> >
> >> >
> >> > On Mon, 29 Nov 2010, Paulius Zaleckas wrote:
> >> >
> >> >> On 11/29/2010 10:02 PM, Arnd Bergmann wrote:
> >> >> > On Monday 29 November 2010 19:52:55 Paulius Zaleckas wrote:
> >> >> > > On 11/29/2010 06:45 PM, Arnd Bergmann wrote:
> >> >> > > > There are many differences between readl and __raw_readl, including
> >> >> > > >
> >> >> > > > * __raw_readl does not have barriers and does not serialize with
> >> >> > > > ? ? spinlocks, so it breaks on out-of-order CPUs.
> >> >> > > > * __raw_readl does not have a specific endianess, while readl is
> >> >> > > > ? ? fixed little-endian, just as the hardware is in this case.
> >> >> > > > ? ? The endian-conversion is a NOP on little-endian ARM, but required
> >> >> > > > ? ? if you actually run on a big-endian ARM (you don't).
> >> >> > > > * __raw_readl may not be atomic, gcc is free to split the access
> >> >> > > > ? ? into byte wise reads (it normally does not, unless you mark
> >> >> > > > ? ? the pointer __attribute__((packed))).
> >> >> > > >
> >> >> > > > In essence, it is almost never a good idea to use __raw_readl, and
> >> >> > > > the double underscores should tell you so.
> >> >> > >
> >> >> > > You are wrong:
> >> >> > >
> >> >> > > Since CONFIG_ARM_DMA_MEM_BUFFERABLE is NOT defined for FA526 core,
> >> >> > > no barriers are in use when using readl. It just translates into
> >> >> > > le32_to_cpu(__raw_readl(x)). Now this CPU has physical pin for endianess
> >> >> > > configuration and if you will chose big-endian you will fail to read
> >> >> > > internal registers, because they ALSO change endianess and le32_to_cpu()
> >> >> > > will screw it. However it is different when accessing registers through
> >> >> > > PCI bus, then you need to use readl().
> >> >> >
> >> >> > Ok, I only checked that the platform does not support big-endian Linux
> >> >> > kernel, not if the HW designers screwed up their registers, sorry about
> >> >> > that.
> >> >> >
> >> >> > The other points are of course still valid: If the code ever gets
> >> >> > used on an out of order CPU, it is broken. More importantly, if someone
> >> >> > looks at the code as an example for writing another PCI support code,
> >> >> > it may end up getting copied to some place where it ends up causing
> >> >> > trouble.
> >> >> >
> >> >> > The typical way to deal with mixed-endian hardware reliably is to have
> >> >> > a header file containing code like
> >> >> >
> >> >> > #ifdef CONFIG_GEMINI_BIG_ENDIAN_IO
> >> >> > #define gemini_readl(x) __swab32(readl(x))
> >> >> > #define ...
> >> >> > #else
> >> >> > #define gemini_readl(x) readl(x))
> >> >> > #endif
> >> >> >
> >> >> > This also takes care of the (not as unlikely as you'd hope) case that
> >> >> > the next person reusing the PCI hardware wires its endianess different
> >> >> > from the CPU endianess.
> >> >>
> >> >> Actually I am not very sure how CPU works in big endian mode :)
> >> >> I have never tried it and I think only some guys who made it did that.
> >> >> So readl will work for 99.99% of cases. In datasheet they say that:
> >> >> "All registers in Gemini use Little Endian and must be accessed by aligned
> >> >> 32-bit word operations. The bus connection interface logic provides an Endian
> >> >> Conversion function."
> >> >> For me it looks like it can mean whatever you want :)
> >> >>
> >> >
> >> > I think the endianes pin switched only the cpu, not the hardware
> >> > registers.
> >>
> >> Yes, but original driver used readl/writel and it does le32_to_cpu,
> >> so that structure definition is just reversing it.
> >> If you will use __raw_readl/__raw_writel than there will be no need
> >> for this redefinition.
> >>
> >> > Here is some sample code from the ethernet devive on Gemini
> >> > typedef union
> >> > {
> >> > ? ? ? ?unsigned int bits32;
> >> > ? ? ? ?struct bit
> >> > ? ? ? ?{
> >> > #if (BIG_ENDIAN==1)
> >> > ? ? ? ? ? ? ? ?unsigned int reserved ? ? ? ? ? : 15; ? // bit 31:17
> >> > ? ? ? ? ? ? ? ?unsigned int v_bit_mode ? ? ? ? : 1; ? ?// bit 16
> >> > ? ? ? ? ? ? ? ?unsigned int device_id ? ? ? ? ?: 12; ? // bit 15:4
> >> > ? ? ? ? ? ? ? ?unsigned int revision_id ? ? ? ?: 4; ? ?// bit ?3:0
> >> > #else
> >> > ? ? ? ? ? ? ? ?unsigned int revision_id ? ? ? ?: 4; ? ?// bit ?3:0
> >> > ? ? ? ? ? ? ? ?unsigned int device_id ? ? ? ? ?: 12; ? // bit 15:4
> >> > ? ? ? ? ? ? ? ?unsigned int v_bit_mode ? ? ? ? : 1; ? ?// bit 16
> >> > ? ? ? ? ? ? ? ?unsigned int reserved ? ? ? ? ? : 15; ? // bit 31:17
> >> > #endif
> >>
> >> The other thing is that this endianess redefinition is very starnge since
> >> it should swap bytes and not bits inside this struct. So I assume that
> >> big endian was never tested on this driver and it will not work.
> >> But ofcouse I can be wrong here :)
> >>
> >
> > At this momment my brain restarts in very slow motion mode ;-)
> > This can't work. The definition Storlinksemi uses for swapping bits and
> > bytes are totaly wrong.
> > They never _even_ testet this, or understand little endian or big endian.
> > Take this simple sample
> >
> > typedef union {
> > ? ? ? ?unsigned int bits32;
> > ? ? ? ?struct bit {
> > #if (BIG_ENDIAN==0)
> > ? ? ? ? ? ? ? ?unsigned int a ?: 1;
> > ? ? ? ? ? ? ? ?unsigned int b ?: 31;
> > #else
> > ? ? ? ? ? ? ? ?unsigned int b ?: 31;
> > ? ? ? ? ? ? ? ?unsigned int a ?: 1;
> > #endif
> > ? ? ? ?};
> > } TEST;
> >
> > They swaped the bits inside one byte
>
> Ha! Wait a minute. Looks like we are both wrong...
> Read the beginning of: http://thread.gmane.org/gmane.linux.kernel/1037608
>
> So it means we should use readl/writel and get rid of these non-portable
> bit-fields...
>

get rid of _endianes_.
Endianes is ordering of bytes (octets) in address space and _not_ bits in
registers nor bytes.

With the above example :
a = 1;
b = 0;

you will get on little endian cpu this
0x01, 0x00, 0x00, 0x00 (0x00000001UL) in memory
on big endian
0x00, 0x00, 0x00, 0x01 (0x01000000UL)
_note_ the bits are not swapped, _only_ bytes.

but if we use this _strange_ register layout with BIG_ENDIAN is set
0x00, 0x00, 0x00, 0x80 (0x80000000UL)

I've done this _on_ paper to understand this.

> >> > ? ? ? ?} bits;
> >> > } TOE_VERSION_T;
> >> >
> >> >
> >> >
> >>
>

2010-12-06 10:52:52

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [PATCH] ARM: Gemini: Add support for PCI BUS

Hello.

On 29-11-2010 19:45, Arnd Bergmann wrote:

>>> The I/O ordering is probably not what you think it is.
>>> There is no ordering guarantee between __raw_writel and
>>> spin_lock/spin_unlock, so you really should be using
>>> readl/writel.

>> No he really should NOT use readl/writel. The ONLY difference
>> between readl/writel and __raw_readl/__raw_writel is endianess
>> conversion. __raw_*l is not doing it. Which to use depend only
>> on HW.

> There are many differences between readl and __raw_readl, including

> * __raw_readl does not have barriers and does not serialize with
> spinlocks, so it breaks on out-of-order CPUs.
> * __raw_readl does not have a specific endianess, while readl is
> fixed little-endian,

I know I'm late but readl()/writel() are CPU-endian, not LE.

WBR, Sergei

2010-12-06 12:20:23