[PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

in(out)_8/in(out)_be16/in(out)_le16 are very powerpc/microblaze
specific. To enable use of Xilinx System ACE driver build for other
architectures (for example it's possible to use it on Xilinx ml-509
board with ARC700 in FPGA) we need to use generic implementation of
accessors.

Current implementation was successfully built with Sourcery G++ Lite
2011.03-39 for Power EABI (ppc44x_defconfig).

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
---
 drivers/block/xsysace.c |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 1f38643..64fd3c0 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -232,14 +232,14 @@ struct ace_reg_ops {
 static u16 ace_in_8(struct ace_device *ace, int reg)
 {
 	void __iomem *r = ace->baseaddr + reg;
-	return in_8(r) | (in_8(r + 1) << 8);
+	return ioread8(r) | (ioread8(r + 1) << 8);
 }
 
 static void ace_out_8(struct ace_device *ace, int reg, u16 val)
 {
 	void __iomem *r = ace->baseaddr + reg;
-	out_8(r, val);
-	out_8(r + 1, val >> 8);
+	iowrite8(val, r);
+	iowrite8(val >> 8, r + 1);
 }
 
 static void ace_datain_8(struct ace_device *ace)
@@ -248,7 +248,7 @@ static void ace_datain_8(struct ace_device *ace)
 	u8 *dst = ace->data_ptr;
 	int i = ACE_FIFO_SIZE;
 	while (i--)
-		*dst++ = in_8(r++);
+		*dst++ = ioread8(r++);
 	ace->data_ptr = dst;
 }
 
@@ -258,7 +258,7 @@ static void ace_dataout_8(struct ace_device *ace)
 	u8 *src = ace->data_ptr;
 	int i = ACE_FIFO_SIZE;
 	while (i--)
-		out_8(r++, *src++);
+		iowrite8(*src++, r++);
 	ace->data_ptr = src;
 }
 
@@ -272,12 +272,12 @@ static struct ace_reg_ops ace_reg_8_ops = {
 /* 16 bit big endian bus attachment */
 static u16 ace_in_be16(struct ace_device *ace, int reg)
 {
-	return in_be16(ace->baseaddr + reg);
+	return ioread16be(ace->baseaddr + reg);
 }
 
 static void ace_out_be16(struct ace_device *ace, int reg, u16 val)
 {
-	out_be16(ace->baseaddr + reg, val);
+	iowrite16be(val, ace->baseaddr + reg);
 }
 
 static void ace_datain_be16(struct ace_device *ace)
@@ -285,7 +285,7 @@ static void ace_datain_be16(struct ace_device *ace)
 	int i = ACE_FIFO_SIZE / 2;
 	u16 *dst = ace->data_ptr;
 	while (i--)
-		*dst++ = in_le16(ace->baseaddr + 0x40);
+		*dst++ = ioread16(ace->baseaddr + 0x40);
 	ace->data_ptr = dst;
 }
 
@@ -294,19 +294,19 @@ static void ace_dataout_be16(struct ace_device *ace)
 	int i = ACE_FIFO_SIZE / 2;
 	u16 *src = ace->data_ptr;
 	while (i--)
-		out_le16(ace->baseaddr + 0x40, *src++);
+		iowrite16(*src++, ace->baseaddr + 0x40);
 	ace->data_ptr = src;
 }
 
 /* 16 bit little endian bus attachment */
 static u16 ace_in_le16(struct ace_device *ace, int reg)
 {
-	return in_le16(ace->baseaddr + reg);
+	return ioread16(ace->baseaddr + reg);
 }
 
 static void ace_out_le16(struct ace_device *ace, int reg, u16 val)
 {
-	out_le16(ace->baseaddr + reg, val);
+	iowrite16(val, ace->baseaddr + reg);
 }
 
 static void ace_datain_le16(struct ace_device *ace)
@@ -314,7 +314,7 @@ static void ace_datain_le16(struct ace_device *ace)
 	int i = ACE_FIFO_SIZE / 2;
 	u16 *dst = ace->data_ptr;
 	while (i--)
-		*dst++ = in_be16(ace->baseaddr + 0x40);
+		*dst++ = ioread16be(ace->baseaddr + 0x40);
 	ace->data_ptr = dst;
 }
 
@@ -323,7 +323,7 @@ static void ace_dataout_le16(struct ace_device *ace)
 	int i = ACE_FIFO_SIZE / 2;
 	u16 *src = ace->data_ptr;
 	while (i--)
-		out_be16(ace->baseaddr + 0x40, *src++);
+		iowrite16be(*src++, ace->baseaddr + 0x40);
 	ace->data_ptr = src;
 }
 
-- 
1.7.10.4

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Tuesday 29 January 2013, Alexey Brodkin wrote:
> in(out)_8/in(out)_be16/in(out)_le16 are very powerpc/microblaze
> specific. To enable use of Xilinx System ACE driver build for other
> architectures (for example it's possible to use it on Xilinx ml-509
> board with ARC700 in FPGA) we need to use generic implementation of
> accessors.
> 
> Current implementation was successfully built with Sourcery G++ Lite
> 2011.03-39 for Power EABI (ppc44x_defconfig).
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

Is this driver used on powerpc64 as well, or just on microblaze
and/or 32 bit powerpc?

On 64 bit powerpc, ioread involves extra overhead because it
goes through the PCI error handling implementation, so we should
keep using in_le() there.	

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 01/29/2013 08:27 PM, Arnd Bergmann wrote:
> On Tuesday 29 January 2013, Alexey Brodkin wrote:
>> in(out)_8/in(out)_be16/in(out)_le16 are very powerpc/microblaze
>> specific. To enable use of Xilinx System ACE driver build for other
>> architectures (for example it's possible to use it on Xilinx ml-509
>> board with ARC700 in FPGA) we need to use generic implementation of
>> accessors.
>>
>> Current implementation was successfully built with Sourcery G++ Lite
>> 2011.03-39 for Power EABI (ppc44x_defconfig).
>>
>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>
> Is this driver used on powerpc64 as well, or just on microblaze
> and/or 32 bit powerpc?
>
> On 64 bit powerpc, ioread involves extra overhead because it
> goes through the PCI error handling implementation, so we should
> keep using in_le() there.	
>
> 	Arnd
>

Personally I have no idea about usage of the named device on powerpc64.
Wondering if anybody may comment on this?

My only intention was to make the driver portable.
Do you think if there's another generic alternative for originally used 
accessors?

For example will it be better with "readb/readw/writeb/writew"?

-Alexey

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/1/30 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
> On 01/29/2013 08:27 PM, Arnd Bergmann wrote:
>>
>> On Tuesday 29 January 2013, Alexey Brodkin wrote:
>>>
>>> in(out)_8/in(out)_be16/in(out)_le16 are very powerpc/microblaze
>>> specific. To enable use of Xilinx System ACE driver build for other
>>> architectures (for example it's possible to use it on Xilinx ml-509
>>> board with ARC700 in FPGA) we need to use generic implementation of
>>> accessors.
>>>
>>> Current implementation was successfully built with Sourcery G++ Lite
>>> 2011.03-39 for Power EABI (ppc44x_defconfig).
>>>
>>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>>
>>
>> Is this driver used on powerpc64 as well, or just on microblaze
>> and/or 32 bit powerpc?
>>
>> On 64 bit powerpc, ioread involves extra overhead because it
>> goes through the PCI error handling implementation, so we should
>> keep using in_le() there.
>>
>>         Arnd
>>
>
> Personally I have no idea about usage of the named device on powerpc64.
> Wondering if anybody may comment on this?
>
> My only intention was to make the driver portable.
> Do you think if there's another generic alternative for originally used
> accessors?
>
> For example will it be better with "readb/readw/writeb/writew"?

This driver is used on ppc32, microblaze little and big endian
and probably could be used on arm le.

Not sure if we can find out proper IO function which we should use.

btw: I will have to support all these combinations for uartlite, xilinx_spi,
gpio and others xilinx IPs.

Thanks,
Michal




-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Vineet Gupta]

On Wednesday 30 January 2013 04:43 PM, Michal Simek wrote:
> 2013/1/30 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
>> On 01/29/2013 08:27 PM, Arnd Bergmann wrote:
>>> On Tuesday 29 January 2013, Alexey Brodkin wrote:
>>>> in(out)_8/in(out)_be16/in(out)_le16 are very powerpc/microblaze
>>>> specific. To enable use of Xilinx System ACE driver build for other
>>>> architectures (for example it's possible to use it on Xilinx ml-509
>>>> board with ARC700 in FPGA) we need to use generic implementation of
>>>> accessors.
>>>>
>>>> Current implementation was successfully built with Sourcery G++ Lite
>>>> 2011.03-39 for Power EABI (ppc44x_defconfig).
>>>>
>>>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>>>
>>> Is this driver used on powerpc64 as well, or just on microblaze
>>> and/or 32 bit powerpc?
>>>
>>> On 64 bit powerpc, ioread involves extra overhead because it
>>> goes through the PCI error handling implementation, so we should
>>> keep using in_le() there.
>>>
>>>         Arnd
>>>
>> Personally I have no idea about usage of the named device on powerpc64.
>> Wondering if anybody may comment on this?
>>
>> My only intention was to make the driver portable.
>> Do you think if there's another generic alternative for originally used
>> accessors?
>>
>> For example will it be better with "readb/readw/writeb/writew"?
> This driver is used on ppc32, microblaze little and big endian
> and probably could be used on arm le.

We intend to use it on ARC architecture, for the Xilinx ML509 boards we use. ARC
port doesn't yet have the special I/O accessors this driver uses. While I can
easily add them to ARC port, it would be better to make this driver portable. This
either means replacing the special I/O accessors the driver uses with what we have
now (if possible on arches) or else provide a default implementation for these
special accessors in asm-generic. I'm up for either one although first approach is
preferable as it avoid adding another set to the zoo of accessors we have right now.

But just adding the accessors to ARC port is a band-aid IMHO.

> Not sure if we can find out proper IO function which we should use.

Is it because of the intricacies of I/O on the arches it uses or because we don't
have well defined semantics of what read/ioread friends should do - or yet
something else.

> btw: I will have to support all these combinations for uartlite, xilinx_spi,
> gpio and others xilinx IPs.

Which further justifies solving this is a proper way anyways.

Thx,
-Vineet

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/1/30 Vineet Gupta <Vineet.Gupta1@synopsys.com>:
> On Wednesday 30 January 2013 04:43 PM, Michal Simek wrote:
>> 2013/1/30 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
>>> On 01/29/2013 08:27 PM, Arnd Bergmann wrote:
>>>> On Tuesday 29 January 2013, Alexey Brodkin wrote:
>>>>> in(out)_8/in(out)_be16/in(out)_le16 are very powerpc/microblaze
>>>>> specific. To enable use of Xilinx System ACE driver build for other
>>>>> architectures (for example it's possible to use it on Xilinx ml-509
>>>>> board with ARC700 in FPGA) we need to use generic implementation of
>>>>> accessors.
>>>>>
>>>>> Current implementation was successfully built with Sourcery G++ Lite
>>>>> 2011.03-39 for Power EABI (ppc44x_defconfig).
>>>>>
>>>>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>>>>
>>>> Is this driver used on powerpc64 as well, or just on microblaze
>>>> and/or 32 bit powerpc?
>>>>
>>>> On 64 bit powerpc, ioread involves extra overhead because it
>>>> goes through the PCI error handling implementation, so we should
>>>> keep using in_le() there.
>>>>
>>>>         Arnd
>>>>
>>> Personally I have no idea about usage of the named device on powerpc64.
>>> Wondering if anybody may comment on this?
>>>
>>> My only intention was to make the driver portable.
>>> Do you think if there's another generic alternative for originally used
>>> accessors?
>>>
>>> For example will it be better with "readb/readw/writeb/writew"?
>> This driver is used on ppc32, microblaze little and big endian
>> and probably could be used on arm le.
>
> We intend to use it on ARC architecture, for the Xilinx ML509 boards we use. ARC
> port doesn't yet have the special I/O accessors this driver uses. While I can
> easily add them to ARC port, it would be better to make this driver portable. This
> either means replacing the special I/O accessors the driver uses with what we have
> now (if possible on arches) or else provide a default implementation for these
> special accessors in asm-generic. I'm up for either one although first approach is
> preferable as it avoid adding another set to the zoo of accessors we have right now.
>
> But just adding the accessors to ARC port is a band-aid IMHO.
>
>> Not sure if we can find out proper IO function which we should use.
>
> Is it because of the intricacies of I/O on the arches it uses or because we don't
> have well defined semantics of what read/ioread friends should do - or yet
> something else.

The problem is mainly for ppc and arm. (mb is ok because of little
endian implementation)

In the driver for arm you can't simple use ioread16be because it
should be little endian
function. And vice versa.
Also from my understanding of arm we should use readl/b/w functions because
they have memory barriers which should be probably performed.

And I haven't found any IO function which will behave on arm as LE and
on PPC as BE.

Maybe you have some suggestion how to solve it.
Look at this thread "Using IO functions across ARM, PPC and Microblaze
architectures"

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Wednesday 30 January 2013 13:31:58 Michal Simek wrote:
> Also from my understanding of arm we should use readl/b/w functions because
> they have memory barriers which should be probably performed.
> 
> And I haven't found any IO function which will behave on arm as LE and
> on PPC as BE.

There are none, except for the __raw_ versions that are generally
not recommended for other reasons.

There really is no easy solution, because most of the hardware IP
blocks have fixed endianess, e.g. an OHCI USB controller will normally
have little-endian registers on all architectures, but in very rare
cases it may be big-endian, because some hardware designer tried to
be helpful and put a byte swapping logic in front of it.

There are a few devices that tend to have the same endianess as the
CPU, again because some hardware designer tried to make things easy
for software by making the hardware confiurable. Again, what normally
happens is that the next hardware designer takes this block and 
hardwires it to some endianess that he sees as "correct" but puts
it on a system with a variable-endian CPU. Note that there are both
little-endian PowerPC systems and big-endian ARM systems, they just
happen to be the minority on an architecture where 99% of the users
prefer the opposite.

The outcome is basically you're screwed for every driver that is
not fixed endianess. The normal workaround is to do something like

#if defined(CONFIG_FOO_BIG_ENDIAN) && !defined(CONFIG_FOO_LITTLE_ENDIAN)
/* always big-endian
#define foo_readl(dev, reg) ioread32_be(dev->regs + reg)
#define foo_writel(dev, reg, val) iowrite32_be(val, dev->regs + reg)
#elif !defined(CONFIG_FOO_BIG_ENDIAN) && defined(CONFIG_FOO_LITTLE_ENDIAN)
/* always little-endian
#define foo_readl(dev, reg) ioread32(dev->regs + reg)
#define foo_writel(dev, reg, val) iowrite32e(val, dev->regs + reg)
#else
/* run-time configured */
#define foo_readl(dev, reg) dev->readl(dev->regs + reg)
#define foo_writel(dev, reg, val) dev->writel(val, dev->regs + reg)
#endif

and select the CONFIG_FOO_BIG_ENDIAN and CONFIG_FOO_LITTLE_ENDIAN
symbols in Kconfig based on the system you are building for.

This of course gets further complicated if you require different
accessors per architecture, like ARM wanting readl or ioread32
and PowerPC wanting in_le32 for a little-endian SoC component.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

HI, [cc: Alan and Geert and David]


2013/1/30 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 30 January 2013 13:31:58 Michal Simek wrote:
>> Also from my understanding of arm we should use readl/b/w functions because
>> they have memory barriers which should be probably performed.
>>
>> And I haven't found any IO function which will behave on arm as LE and
>> on PPC as BE.
>
> There are none, except for the __raw_ versions that are generally
> not recommended for other reasons.
>
> There really is no easy solution, because most of the hardware IP
> blocks have fixed endianess, e.g. an OHCI USB controller will normally
> have little-endian registers on all architectures, but in very rare
> cases it may be big-endian, because some hardware designer tried to
> be helpful and put a byte swapping logic in front of it.
>
> There are a few devices that tend to have the same endianess as the
> CPU, again because some hardware designer tried to make things easy
> for software by making the hardware confiurable. Again, what normally
> happens is that the next hardware designer takes this block and
> hardwires it to some endianess that he sees as "correct" but puts
> it on a system with a variable-endian CPU. Note that there are both
> little-endian PowerPC systems and big-endian ARM systems, they just
> happen to be the minority on an architecture where 99% of the users
> prefer the opposite.
>
> The outcome is basically you're screwed for every driver that is
> not fixed endianess. The normal workaround is to do something like
>
> #if defined(CONFIG_FOO_BIG_ENDIAN) && !defined(CONFIG_FOO_LITTLE_ENDIAN)
> /* always big-endian
> #define foo_readl(dev, reg) ioread32_be(dev->regs + reg)
> #define foo_writel(dev, reg, val) iowrite32_be(val, dev->regs + reg)
> #elif !defined(CONFIG_FOO_BIG_ENDIAN) && defined(CONFIG_FOO_LITTLE_ENDIAN)
> /* always little-endian
> #define foo_readl(dev, reg) ioread32(dev->regs + reg)
> #define foo_writel(dev, reg, val) iowrite32e(val, dev->regs + reg)
> #else
> /* run-time configured */
> #define foo_readl(dev, reg) dev->readl(dev->regs + reg)
> #define foo_writel(dev, reg, val) dev->writel(val, dev->regs + reg)
> #endif
>
> and select the CONFIG_FOO_BIG_ENDIAN and CONFIG_FOO_LITTLE_ENDIAN
> symbols in Kconfig based on the system you are building for.

Using CONFIG_FOO_BIG/LITTLE is not good because it is just another
Kconfig option.
You can easily detect it at runtime and for dedicated hw with fixed
endians you can just
handle it in the driver and don't care about global setting.


> This of course gets further complicated if you require different
> accessors per architecture, like ARM wanting readl or ioread32
> and PowerPC wanting in_le32 for a little-endian SoC component.

FYI: I have got two responses on linux-arch from Alan
"Set the pointers up and pass them as data with your platform device, that
way the function definitions are buried in your platform code where they
depend."

and Geert:
"Or embed a struct io_ops * in struct device, to be set up by the bus driver?

Wasn't David Hinds working on something like this in the context of PCMCIA
a few decades ago?"

Based on their suggestions one way can be to pass it through void *platform_data
which is probably not the best and then which make more sense to me is to extend
struct dev_archdata archdata to add there native read/write functions.

What do you think?

Thanks,
Michal



-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Monday 04 February 2013, Michal Simek wrote:
> >
> > and select the CONFIG_FOO_BIG_ENDIAN and CONFIG_FOO_LITTLE_ENDIAN
> > symbols in Kconfig based on the system you are building for.
> 
> Using CONFIG_FOO_BIG/LITTLE is not good because it is just another
> Kconfig option.
> You can easily detect it at runtime and for dedicated hw with fixed
> endians you can just
> handle it in the driver and don't care about global setting.

The configuration option should not be visible, so it's
not something a user would have to worry about. It's just
sometimes nicer to express the configuration of the platform
in terms of Kconfig syntax than it is in C code if you have
complex platform dependencies.

> > This of course gets further complicated if you require different
> > accessors per architecture, like ARM wanting readl or ioread32
> > and PowerPC wanting in_le32 for a little-endian SoC component.
> 
> FYI: I have got two responses on linux-arch from Alan
> "Set the pointers up and pass them as data with your platform device, that
> way the function definitions are buried in your platform code where they
> depend."
> 
> and Geert:
> "Or embed a struct io_ops * in struct device, to be set up by the bus driver?
> 
> Wasn't David Hinds working on something like this in the context of PCMCIA
> a few decades ago?"
> 
> Based on their suggestions one way can be to pass it through void *platform_data
> which is probably not the best and then which make more sense to me is to extend
> struct dev_archdata archdata to add there native read/write functions.
> 
> What do you think?

I worry a little about code size if we have a lot of drivers that go
from one instruction to an indirect function call for each readl/writel.
Using platform_data ss also something that does not work too well with
device tree based platform configuration, which tries hard to leave
all run-time configuration inside of the driver.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Mon, 2013-02-04 at 17:24 +0000, Arnd Bergmann wrote:
> On Monday 04 February 2013, Michal Simek wrote:
> > >
> > > and select the CONFIG_FOO_BIG_ENDIAN and CONFIG_FOO_LITTLE_ENDIAN
> > > symbols in Kconfig based on the system you are building for.
> > 
> > Using CONFIG_FOO_BIG/LITTLE is not good because it is just another
> > Kconfig option.
> > You can easily detect it at runtime and for dedicated hw with fixed
> > endians you can just
> > handle it in the driver and don't care about global setting.
> 
> The configuration option should not be visible, so it's
> not something a user would have to worry about. It's just
> sometimes nicer to express the configuration of the platform
> in terms of Kconfig syntax than it is in C code if you have
> complex platform dependencies.

As long as you never have a case where both are needed at runtime...

> > > This of course gets further complicated if you require different
> > > accessors per architecture, like ARM wanting readl or ioread32
> > > and PowerPC wanting in_le32 for a little-endian SoC component.

powerpc should never "want" in_le32. ioread32 should work just as well.
in_le32 is historical stuff and is never required.
 
> > FYI: I have got two responses on linux-arch from Alan
> > "Set the pointers up and pass them as data with your platform device, that
> > way the function definitions are buried in your platform code where they
> > depend."
> > 
> > and Geert:
> > "Or embed a struct io_ops * in struct device, to be set up by the bus driver?
> > 
> > Wasn't David Hinds working on something like this in the context of PCMCIA
> > a few decades ago?"
> > 
> > Based on their suggestions one way can be to pass it through void *platform_data
> > which is probably not the best and then which make more sense to me is to extend
> > struct dev_archdata archdata to add there native read/write functions.
> > 
> > What do you think?
> 
> I worry a little about code size if we have a lot of drivers that go
> from one instruction to an indirect function call for each readl/writel.
> Using platform_data ss also something that does not work too well with
> device tree based platform configuration, which tries hard to leave
> all run-time configuration inside of the driver.

readl/writel and ioread32/iowrite32 should always be equivalent.

So it all boils down to whether your device has different endianness (it
should not, doing so is stupid ... but if it really does then make it a
runtime wrapper that chooses between ioread32 and ioread32be

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/5 Benjamin Herrenschmidt <benh@kernel.crashing.org>:
> On Mon, 2013-02-04 at 17:24 +0000, Arnd Bergmann wrote:
>> On Monday 04 February 2013, Michal Simek wrote:
>> > >
>> > > and select the CONFIG_FOO_BIG_ENDIAN and CONFIG_FOO_LITTLE_ENDIAN
>> > > symbols in Kconfig based on the system you are building for.
>> >
>> > Using CONFIG_FOO_BIG/LITTLE is not good because it is just another
>> > Kconfig option.
>> > You can easily detect it at runtime and for dedicated hw with fixed
>> > endians you can just
>> > handle it in the driver and don't care about global setting.
>>
>> The configuration option should not be visible, so it's
>> not something a user would have to worry about. It's just
>> sometimes nicer to express the configuration of the platform
>> in terms of Kconfig syntax than it is in C code if you have
>> complex platform dependencies.
>
> As long as you never have a case where both are needed at runtime...
>
>> > > This of course gets further complicated if you require different
>> > > accessors per architecture, like ARM wanting readl or ioread32
>> > > and PowerPC wanting in_le32 for a little-endian SoC component.
>
> powerpc should never "want" in_le32. ioread32 should work just as well.
> in_le32 is historical stuff and is never required.
>
>> > FYI: I have got two responses on linux-arch from Alan
>> > "Set the pointers up and pass them as data with your platform device, that
>> > way the function definitions are buried in your platform code where they
>> > depend."
>> >
>> > and Geert:
>> > "Or embed a struct io_ops * in struct device, to be set up by the bus driver?
>> >
>> > Wasn't David Hinds working on something like this in the context of PCMCIA
>> > a few decades ago?"
>> >
>> > Based on their suggestions one way can be to pass it through void *platform_data
>> > which is probably not the best and then which make more sense to me is to extend
>> > struct dev_archdata archdata to add there native read/write functions.
>> >
>> > What do you think?
>>
>> I worry a little about code size if we have a lot of drivers that go
>> from one instruction to an indirect function call for each readl/writel.
>> Using platform_data ss also something that does not work too well with
>> device tree based platform configuration, which tries hard to leave
>> all run-time configuration inside of the driver.
>
> readl/writel and ioread32/iowrite32 should always be equivalent.
>
> So it all boils down to whether your device has different endianness (it
> should not, doing so is stupid ... but if it really does then make it a
> runtime wrapper that chooses between ioread32 and ioread32be

xilinx ppc is big endian
xilinx arm is little endian
xilinx microblaze is little endian and big endian

It is just sharing the same IP across all platforms. Which is better
than create new devices and new device drivers for it. It means that
all of them are register compatible but require access with native
platform endianness
as I listed above.

It is not a problem to create runtime wrapper and even detect endian
directly in the driver
but the point if this is the proper design.
Also ioread32 and ioread32be shouldn't be used on ARM because there
are missing memory barriers.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Vineet Gupta]

Hi Michal, Arnd, Ben

On Tuesday 05 February 2013 04:24 PM, Michal Simek wrote:
> 2013/2/5 Benjamin Herrenschmidt <benh@kernel.crashing.org>:
>> On Mon, 2013-02-04 at 17:24 +0000, Arnd Bergmann wrote:
>>> On Monday 04 February 2013, Michal Simek wrote:
>>>>> and select the CONFIG_FOO_BIG_ENDIAN and CONFIG_FOO_LITTLE_ENDIAN
>>>>> symbols in Kconfig based on the system you are building for.
>>>> Using CONFIG_FOO_BIG/LITTLE is not good because it is just another
>>>> Kconfig option.
>>>> You can easily detect it at runtime and for dedicated hw with fixed
>>>> endians you can just
>>>> handle it in the driver and don't care about global setting.
>>> The configuration option should not be visible, so it's
>>> not something a user would have to worry about. It's just
>>> sometimes nicer to express the configuration of the platform
>>> in terms of Kconfig syntax than it is in C code if you have
>>> complex platform dependencies.
>> As long as you never have a case where both are needed at runtime...
>>
>>>>> This of course gets further complicated if you require different
>>>>> accessors per architecture, like ARM wanting readl or ioread32
>>>>> and PowerPC wanting in_le32 for a little-endian SoC component.
>> powerpc should never "want" in_le32. ioread32 should work just as well.
>> in_le32 is historical stuff and is never required.
>>
>>>> FYI: I have got two responses on linux-arch from Alan
>>>> "Set the pointers up and pass them as data with your platform device, that
>>>> way the function definitions are buried in your platform code where they
>>>> depend."
>>>>
>>>> and Geert:
>>>> "Or embed a struct io_ops * in struct device, to be set up by the bus driver?
>>>>
>>>> Wasn't David Hinds working on something like this in the context of PCMCIA
>>>> a few decades ago?"
>>>>
>>>> Based on their suggestions one way can be to pass it through void *platform_data
>>>> which is probably not the best and then which make more sense to me is to extend
>>>> struct dev_archdata archdata to add there native read/write functions.
>>>>
>>>> What do you think?
>>> I worry a little about code size if we have a lot of drivers that go
>>> from one instruction to an indirect function call for each readl/writel.
>>> Using platform_data ss also something that does not work too well with
>>> device tree based platform configuration, which tries hard to leave
>>> all run-time configuration inside of the driver.
>> readl/writel and ioread32/iowrite32 should always be equivalent.
>>
>> So it all boils down to whether your device has different endianness (it
>> should not, doing so is stupid ... but if it really does then make it a
>> runtime wrapper that chooses between ioread32 and ioread32be
> xilinx ppc is big endian
> xilinx arm is little endian
> xilinx microblaze is little endian and big endian
>
> It is just sharing the same IP across all platforms. Which is better
> than create new devices and new device drivers for it. It means that
> all of them are register compatible but require access with native
> platform endianness
> as I listed above.
>
> It is not a problem to create runtime wrapper and even detect endian
> directly in the driver
> but the point if this is the proper design.
> Also ioread32 and ioread32be shouldn't be used on ARM because there
> are missing memory barriers.

I'm sorry, despite watching Ben's LPC talk
(http://linuxplumbers.ubicast.tv/videos/big-and-little-endian-inside-out/) and
trying to following the other discussions on this topic, I'm still not quite
following the real issue here. So I'll ask a noob question despite the hazard of
sounding stupid.

Parking the ioread32 vs readl issue for a moment, asm-generic/io.h (for
!CONFIG_GENERIC_IOMAP) is as follows

#define ioread32(addr)        readl(addr)
#define ioread32be(addr)    be32_to_cpu(ioread32(addr))

Am I correct in understanding that "be" suffix here reflects the byte ordering of
the device and not the CPU. So for a BE device, driver using ioread32be on LE
processor will get MSB first data - and accessor will swap the bytes to make it
lsb first in register, while on BE processor, the lanes to memory are swapped,
causing the msb first word to be flipped before ending up in the cpu register - so
in the end, CPU register on either will have the exact same value. Correct ?

Thx,
-Vineet

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Tue, 2013-02-05 at 17:39 +0530, Vineet Gupta wrote:
> Am I correct in understanding that "be" suffix here reflects the byte ordering of
> the device and not the CPU. So for a BE device, driver using ioread32be on LE
> processor will get MSB first data - and accessor will swap the bytes to make it
> lsb first in register, while on BE processor, the lanes to memory are swapped,
> causing the msb first word to be flipped before ending up in the cpu register - so
> in the end, CPU register on either will have the exact same value. Correct ?

Yes. That's how ioread32 and ioread32be are defined. They relate to the device
endianness regardless of the CPU endianness.

ioread32 -> little endian device
ioread32be -> big endian device

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Tue, 2013-02-05 at 11:54 +0100, Michal Simek wrote:
> 
> xilinx ppc is big endian
> xilinx arm is little endian
> xilinx microblaze is little endian and big endian

You are talking about the core, why should the xsysace block driver be
synthesized with the same endianness as the core ? That's not terribly
useful, especially since reverse load/stores on ppc are essentially
free...

> It is just sharing the same IP across all platforms. Which is better
> than create new devices and new device drivers for it. It means that
> all of them are register compatible but require access with native
> platform endianness as I listed above.

Every attempt at doing "native platform endianness" has always been a
misguided attempt turning into a trainwreck (see OHCI USB).

Just pick one endian for the device and stick to it.

> It is not a problem to create runtime wrapper and even detect endian
> directly in the driver
> but the point if this is the proper design.
> Also ioread32 and ioread32be shouldn't be used on ARM because there
> are missing memory barriers.

Then fix them, they shouldn't be, it's a bug, it will break many other
drivers. They should be fully equivalent to readl.

Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/5 Benjamin Herrenschmidt <benh@kernel.crashing.org>:
> On Tue, 2013-02-05 at 11:54 +0100, Michal Simek wrote:
>>
>> xilinx ppc is big endian
>> xilinx arm is little endian
>> xilinx microblaze is little endian and big endian
>
> You are talking about the core, why should the xsysace block driver be
> synthesized with the same endianness as the core ? That's not terribly
> useful, especially since reverse load/stores on ppc are essentially
> free...

It is done by hardware guys and I can do nothing with it.

>> It is just sharing the same IP across all platforms. Which is better
>> than create new devices and new device drivers for it. It means that
>> all of them are register compatible but require access with native
>> platform endianness as I listed above.
>
> Every attempt at doing "native platform endianness" has always been a
> misguided attempt turning into a trainwreck (see OHCI USB).
>
> Just pick one endian for the device and stick to it.

It is reality and I can't change it. Arnd mentioned it earlier that USB


>> It is not a problem to create runtime wrapper and even detect endian
>> directly in the driver
>> but the point if this is the proper design.
>> Also ioread32 and ioread32be shouldn't be used on ARM because there
>> are missing memory barriers.
>
> Then fix them, they shouldn't be, it's a bug, it will break many other
> drivers. They should be fully equivalent to readl.

I want to be sure about this. I have parsed this again with closer look and
seems to me that ioread32 is equal to readl and iowrite32 to writel.
Arnd: Am I right?

Thanks,
Michal






-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/05/2013 04:38 PM, Michal Simek wrote:
> 2013/2/5 Benjamin Herrenschmidt <benh@kernel.crashing.org>:
>> On Tue, 2013-02-05 at 11:54 +0100, Michal Simek wrote:
>>>
>>> xilinx ppc is big endian
>>> xilinx arm is little endian
>>> xilinx microblaze is little endian and big endian
>>
>> You are talking about the core, why should the xsysace block driver be
>> synthesized with the same endianness as the core ? That's not terribly
>> useful, especially since reverse load/stores on ppc are essentially
>> free...
>
> It is done by hardware guys and I can do nothing with it.

 From this document 
(http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf) I 
can tell that Xilinx System ACE CF solution is a chip manufactured in 
silicon, here's an extract:
=====
As shown in Figure 1, the System ACE CompactFlash solution is a chipset, 
consisting of a controller device (System
ACE CF controller) and a commercially available CompactFlash storage 
device.
=====

and its endianess is fixed which is stated in this document 
(http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf).
=====
The Xilinx System ACE Compact Flash chip is a true little-endian device
=====

But in its turn Xilinx System ACE Compact Flash chip is attached to 
CPU's interface bus via some bridge. Depending on interface bus used it 
could be BVCI-to-MPU (Microprocessor Interface of the System ACE
Compact Flash solution peripheral) for ARC, PLB for MicroBlaze etc.
And this bridge in general may do whatever its (HW) developer wants.

For example for MicroBlaze Xilinx has its XPS Sysace interface 
controller 
(http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf) 
which does bit-swapping from PLB's big-endian bytes to xsysace 
CF-controller little-endian bytes (though bytes in words are not swapped):
=====
The Xilinx System ACE Compact Flash chip is a true little-endian device 
and the PLB is a big-endian bus. Therefore the XPS System ACE Interface 
Controller will do a bit-swap in each byte when connecting the PLB data 
bus to the System ACE data bus as shown in Table 2.

Note however, that the XPS System ACE Interface Controller does not 
perform the byte swapping necessary to interface to a little-endian 
device when configured to use 16-bit mode. Therefore, the software 
drivers provided for this core will perform the necessary byte-swapping 
to correctly interface to the Xilinx System ACE Compact Flash chip as 
shown in Table 3.
=====

So at this point I'd say that data access should be done differently 
depending on HW (bridge) used.

Another question is do we know for sure that for particular architecture 
only 1 interface bridge is used. If so then we may select proper 
accessors per architecture.

>
>>> It is just sharing the same IP across all platforms. Which is better
>>> than create new devices and new device drivers for it. It means that
>>> all of them are register compatible but require access with native
>>> platform endianness as I listed above.
>>
>> Every attempt at doing "native platform endianness" has always been a
>> misguided attempt turning into a trainwreck (see OHCI USB).
>>
>> Just pick one endian for the device and stick to it.
>
> It is reality and I can't change it. Arnd mentioned it earlier that USB
>
>
>>> It is not a problem to create runtime wrapper and even detect endian
>>> directly in the driver
>>> but the point if this is the proper design.
>>> Also ioread32 and ioread32be shouldn't be used on ARM because there
>>> are missing memory barriers.
>>
>> Then fix them, they shouldn't be, it's a bug, it will break many other
>> drivers. They should be fully equivalent to readl.
>
> I want to be sure about this. I have parsed this again with closer look and
> seems to me that ioread32 is equal to readl and iowrite32 to writel.
> Arnd: Am I right?
>
> Thanks,
> Michal

-Alexey

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Tuesday 05 February 2013 18:03:31 Alexey Brodkin wrote:
> The Xilinx System ACE Compact Flash chip is a true little-endian device 
> and the PLB is a big-endian bus. Therefore the XPS System ACE Interface 
> Controller will do a bit-swap in each byte when connecting the PLB data 
> bus to the System ACE data bus as shown in Table 2.
> 
> Note however, that the XPS System ACE Interface Controller does not 
> perform the byte swapping necessary to interface to a little-endian 
> device when configured to use 16-bit mode. Therefore, the software 
> drivers provided for this core will perform the necessary byte-swapping 
> to correctly interface to the Xilinx System ACE Compact Flash chip as 
> shown in Table 3.

Ok. In this case, I would recommend making the default for this driver
little-endian, and adding a quirk for broken hardware bridges like the
one you cited to have a mixed-endian mode if configured so at compile
time.

It seems that on all normal platforms, this device should behave as
little-endian, while the Xilinx bridge can be either big-endian
or little-endian, depending on whether it is used in 8-bit or 16-bit
mode, so if we are using this, it cannot be known at compile time.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Tue, 2013-02-05 at 16:12 +0100, Arnd Bergmann wrote:
> Ok. In this case, I would recommend making the default for this driver
> little-endian, and adding a quirk for broken hardware bridges like the
> one you cited to have a mixed-endian mode if configured so at compile
> time.
> 
> It seems that on all normal platforms, this device should behave as
> little-endian, while the Xilinx bridge can be either big-endian
> or little-endian, depending on whether it is used in 8-bit or 16-bit
> mode, so if we are using this, it cannot be known at compile time.

Why ? 8-bit devices shouldn't need anything special. 16-bit should be
wired properly to not need anything special either. Why would we bother
supporting a bad wiring ? Let them feel the pain, with luck it will
provide incentive for them to fix it.

Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Tue, Feb 5, 2013 at 3:12 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 05 February 2013 18:03:31 Alexey Brodkin wrote:
>> The Xilinx System ACE Compact Flash chip is a true little-endian device
>> and the PLB is a big-endian bus. Therefore the XPS System ACE Interface
>> Controller will do a bit-swap in each byte when connecting the PLB data
>> bus to the System ACE data bus as shown in Table 2.
>>
>> Note however, that the XPS System ACE Interface Controller does not
>> perform the byte swapping necessary to interface to a little-endian
>> device when configured to use 16-bit mode. Therefore, the software
>> drivers provided for this core will perform the necessary byte-swapping
>> to correctly interface to the Xilinx System ACE Compact Flash chip as
>> shown in Table 3.
>
> Ok. In this case, I would recommend making the default for this driver
> little-endian, and adding a quirk for broken hardware bridges like the
> one you cited to have a mixed-endian mode if configured so at compile
> time.
>
> It seems that on all normal platforms, this device should behave as
> little-endian, while the Xilinx bridge can be either big-endian
> or little-endian, depending on whether it is used in 8-bit or 16-bit
> mode, so if we are using this, it cannot be known at compile time.

The driver already handles this. It has three sets of accessors;
8-bit, 16-bit LE and 16-bit BE *and* when doing 16-bit it figures out
on its own which set to use at runtime. There is nothing controversial
here. The only problem is that the driver is currently using in_/out_
IO accessors instead of ioread/iowrite variants.

g.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Wednesday 06 February 2013, Grant Likely wrote:
> The driver already handles this. It has three sets of accessors;
> 8-bit, 16-bit LE and 16-bit BE and when doing 16-bit it figures out
> on its own which set to use at runtime. There is nothing controversial
> here. The only problem is that the driver is currently using in_/out_
> IO accessors instead of ioread/iowrite variants.

Ah right. It certainly helps to look at the source code.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/6 Grant Likely <grant.likely@secretlab.ca>:
> On Tue, Feb 5, 2013 at 3:12 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tuesday 05 February 2013 18:03:31 Alexey Brodkin wrote:
>>> The Xilinx System ACE Compact Flash chip is a true little-endian device
>>> and the PLB is a big-endian bus. Therefore the XPS System ACE Interface
>>> Controller will do a bit-swap in each byte when connecting the PLB data
>>> bus to the System ACE data bus as shown in Table 2.
>>>
>>> Note however, that the XPS System ACE Interface Controller does not
>>> perform the byte swapping necessary to interface to a little-endian
>>> device when configured to use 16-bit mode. Therefore, the software
>>> drivers provided for this core will perform the necessary byte-swapping
>>> to correctly interface to the Xilinx System ACE Compact Flash chip as
>>> shown in Table 3.
>>
>> Ok. In this case, I would recommend making the default for this driver
>> little-endian, and adding a quirk for broken hardware bridges like the
>> one you cited to have a mixed-endian mode if configured so at compile
>> time.
>>
>> It seems that on all normal platforms, this device should behave as
>> little-endian, while the Xilinx bridge can be either big-endian
>> or little-endian, depending on whether it is used in 8-bit or 16-bit
>> mode, so if we are using this, it cannot be known at compile time.
>
> The driver already handles this. It has three sets of accessors;
> 8-bit, 16-bit LE and 16-bit BE *and* when doing 16-bit it figures out
> on its own which set to use at runtime. There is nothing controversial
> here. The only problem is that the driver is currently using in_/out_
> IO accessors instead of ioread/iowrite variants.

I have looked at the patches from more practical side and I have tested it on
microblaze big endian in 16bit mode and I have found that sysace driver
stop to work.
After that I have looked at ioread/iowrite microblaze implementation
and implementation of that functions is wrong.
I have fixed it but looking at using asm-generic/io.h for microblaze.

I will do more tests and let you know.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Wednesday 06 February 2013 17:21:37 Michal Simek wrote:

> I have looked at the patches from more practical side and I have tested it on
> microblaze big endian in 16bit mode and I have found that sysace driver
> stop to work.
> After that I have looked at ioread/iowrite microblaze implementation
> and implementation of that functions is wrong.
> I have fixed it but looking at using asm-generic/io.h for microblaze.
> 
> I will do more tests and let you know.

Well, I think they are only wrong in the way that they ignore
endianess. You can fix that by changing them to be identical
to the in_le/in_be families.

However, I would also recommend changing your __raw_* accessors
to inline assembly functions rather than pointer dereferences,
because we have had problems in the past where gcc (when faced
with undefined C) silently turned 32-bit accesses into multiples
of byte accesses, which can be fatal for MMIO. The asm-generic
version obviously cannot get this right.

The PCI I/O space handling, as mentioned, is completely broken
on microblaze, and you can either use the approach from asm-generic
when you set PCI_IOBASE match your isa_io_base.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

On 02/07/2013 01:34 AM, Arnd Bergmann wrote:
> On Wednesday 06 February 2013 17:21:37 Michal Simek wrote:
>
>> I have looked at the patches from more practical side and I have tested it on
>> microblaze big endian in 16bit mode and I have found that sysace driver
>> stop to work.
>> After that I have looked at ioread/iowrite microblaze implementation
>> and implementation of that functions is wrong.
>> I have fixed it but looking at using asm-generic/io.h for microblaze.
>>
>> I will do more tests and let you know.
>
> Well, I think they are only wrong in the way that they ignore
> endianess. You can fix that by changing them to be identical
> to the in_le/in_be families.

But still it is wrong.

>
> However, I would also recommend changing your __raw_* accessors
> to inline assembly functions rather than pointer dereferences,
> because we have had problems in the past where gcc (when faced
> with undefined C) silently turned 32-bit accesses into multiples
> of byte accesses, which can be fatal for MMIO. The asm-generic
> version obviously cannot get this right.
>
> The PCI I/O space handling, as mentioned, is completely broken
> on microblaze, and you can either use the approach from asm-generic
> when you set PCI_IOBASE match your isa_io_base.

One question regarding to asm-generic/io.h about iowrite16be implementation

#define iowrite16be(v, addr)   iowrite16(be16_to_cpu(v), (addr))
#define iowrite16(v, addr)	writew((v), (addr))
#define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)

static inline void __raw_writew(u16 b, volatile void __iomem *addr)
{
	*(volatile u16 __force *) addr = b;
}

How is this suppose to work on Big Endian?
be16_to_cpu(v) is (v)
and
__cpu_to_le16(b) is swab16(v)

On little
be16_to_cpu(v) is swab16(v)
and
__cpu_to_le16(swab(b)) is swab16(v)


What I would expect is
#define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)

on Big endian:
__cpu_to_be16(v) is (v)
on Little endian:
__cpu_to_be16(v) is swab(v)

What am I missing here?

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Geert Uytterhoeven]

On Wed, Feb 6, 2013 at 6:40 PM, Michal Simek <monstr@monstr.eu> wrote:
> One question regarding to asm-generic/io.h about iowrite16be implementation
>
> #define iowrite16be(v, addr)   iowrite16(be16_to_cpu(v), (addr))
> #define iowrite16(v, addr)      writew((v), (addr))
> #define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
>
> static inline void __raw_writew(u16 b, volatile void __iomem *addr)
> {
>         *(volatile u16 __force *) addr = b;
> }
>
> How is this suppose to work on Big Endian?
> be16_to_cpu(v) is (v)
> and
> __cpu_to_le16(b) is swab16(v)

Yes.

> On little
> be16_to_cpu(v) is swab16(v)

Yes.

> and
> __cpu_to_le16(swab(b)) is swab16(v)

???

Don't you mean "__cpu_to_le16(b) is (b)"?

> What I would expect is
> #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)

Indeed, it should be "__cpu_to_be16(v)" instead of "be16_to_cpu(v)".

Same for iowrite32be().

> on Big endian:
> __cpu_to_be16(v) is (v)
> on Little endian:
> __cpu_to_be16(v) is swab(v)
>
> What am I missing here?

But as both conversions are identical (just swapping 2 bytes), It Just Works.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/6 Geert Uytterhoeven <geert@linux-m68k.org>:
> On Wed, Feb 6, 2013 at 6:40 PM, Michal Simek <monstr@monstr.eu> wrote:
>> One question regarding to asm-generic/io.h about iowrite16be implementation
>>
>> #define iowrite16be(v, addr)   iowrite16(be16_to_cpu(v), (addr))
>> #define iowrite16(v, addr)      writew((v), (addr))
>> #define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
>>
>> static inline void __raw_writew(u16 b, volatile void __iomem *addr)
>> {
>>         *(volatile u16 __force *) addr = b;
>> }
>>
>> How is this suppose to work on Big Endian?
>> be16_to_cpu(v) is (v)
>> and
>> __cpu_to_le16(b) is swab16(v)
>
> Yes.

But on native BE system ( I expect that v is in big endian)
iowrite16be(v, addr) should be just *(volatile u16 __force *) addr =
v; not *(volatile u16 __force *) addr = swab16(v);


>> On little
>> be16_to_cpu(v) is swab16(v)
>
> Yes.
>
>> and
>> __cpu_to_le16(swab(b)) is swab16(v)
>
> ???
>
> Don't you mean "__cpu_to_le16(b) is (b)"?

BTW: I took output value from the first line (be16_to_cpu(v) is swab16(v))

Grrr - on LE this code works. (I expect that v is in little endian)
iowrite16be(v, addr) is *(volatile u16 __force *) addr = swab16(v);


>> What I would expect is
>> #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)
>
> Indeed, it should be "__cpu_to_be16(v)" instead of "be16_to_cpu(v)".

What do you mean by that?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Geert Uytterhoeven]

On Thu, Feb 7, 2013 at 8:23 AM, Michal Simek <monstr@monstr.eu> wrote:
>>> #define iowrite16be(v, addr)   iowrite16(be16_to_cpu(v), (addr))
>>> #define iowrite16(v, addr)      writew((v), (addr))
>>> #define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
>>>
>>> static inline void __raw_writew(u16 b, volatile void __iomem *addr)
>>> {
>>>         *(volatile u16 __force *) addr = b;
>>> }
>>>
>>> How is this suppose to work on Big Endian?
>>> be16_to_cpu(v) is (v)
>>> and
>>> __cpu_to_le16(b) is swab16(v)
>>
>> Yes.
>
> But on native BE system ( I expect that v is in big endian)
> iowrite16be(v, addr) should be just *(volatile u16 __force *) addr =
> v; not *(volatile u16 __force *) addr = swab16(v);

>>> What I would expect is
>>> #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)
>>
>> Indeed, it should be "__cpu_to_be16(v)" instead of "be16_to_cpu(v)".
>
> What do you mean by that?

Bummer, I missed that current iowrite16be() uses (the little endian)
iowrite16(),
not _raw_writew(), and thought the only difference between the original
and your version was the endianness conversion macro.

Yes,

    #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)

should be correct.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

On 02/07/2013 08:38 AM, Geert Uytterhoeven wrote:
> On Thu, Feb 7, 2013 at 8:23 AM, Michal Simek <monstr@monstr.eu> wrote:
>>>> #define iowrite16be(v, addr)   iowrite16(be16_to_cpu(v), (addr))
>>>> #define iowrite16(v, addr)      writew((v), (addr))
>>>> #define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
>>>>
>>>> static inline void __raw_writew(u16 b, volatile void __iomem *addr)
>>>> {
>>>>          *(volatile u16 __force *) addr = b;
>>>> }
>>>>
>>>> How is this suppose to work on Big Endian?
>>>> be16_to_cpu(v) is (v)
>>>> and
>>>> __cpu_to_le16(b) is swab16(v)
>>>
>>> Yes.
>>
>> But on native BE system ( I expect that v is in big endian)
>> iowrite16be(v, addr) should be just *(volatile u16 __force *) addr =
>> v; not *(volatile u16 __force *) addr = swab16(v);
>
>>>> What I would expect is
>>>> #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)
>>>
>>> Indeed, it should be "__cpu_to_be16(v)" instead of "be16_to_cpu(v)".
>>
>> What do you mean by that?
>
> Bummer, I missed that current iowrite16be() uses (the little endian)
> iowrite16(),
> not _raw_writew(), and thought the only difference between the original
> and your version was the endianness conversion macro.
>
> Yes,
>
>      #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)
>
> should be correct.

ok. Can you please confirm with me that the same problem is also for iowrite32be
ioread16be and ioread32be?

This description seems to me correct for BE and LE.
#define ioread16be(addr)       __be16_to_cpu(__raw_readw(addr))
#define ioread32be(addr)       __be32_to_cpu(__raw_readl(addr))
#define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)
#define iowrite32be(v, addr)   __raw_writel(__cpu_to_be32(v), addr)

What do you think?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Geert Uytterhoeven]

On Thu, Feb 7, 2013 at 9:01 AM, Michal Simek <monstr@monstr.eu> wrote:
> ok. Can you please confirm with me that the same problem is also for
> iowrite32be
> ioread16be and ioread32be?
>
> This description seems to me correct for BE and LE.
> #define ioread16be(addr)       __be16_to_cpu(__raw_readw(addr))
> #define ioread32be(addr)       __be32_to_cpu(__raw_readl(addr))
>
> #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)
> #define iowrite32be(v, addr)   __raw_writel(__cpu_to_be32(v), addr)
>
> What do you think?

Looks fine to me. Arnd, Ben?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Thursday 07 February 2013, Geert Uytterhoeven wrote:
> 
> On Thu, Feb 7, 2013 at 9:01 AM, Michal Simek <monstr@monstr.eu> wrote:
> > ok. Can you please confirm with me that the same problem is also for
> > iowrite32be
> > ioread16be and ioread32be?
> >
> > This description seems to me correct for BE and LE.
> > #define ioread16be(addr)       __be16_to_cpu(__raw_readw(addr))
> > #define ioread32be(addr)       __be32_to_cpu(__raw_readl(addr))
> >
> > #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)
> > #define iowrite32be(v, addr)   __raw_writel(__cpu_to_be32(v), addr)
> >
> > What do you think?
> 
> Looks fine to me. Arnd, Ben?

Yes, that would be better. Can you prepare a patch?

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/7 Arnd Bergmann <arnd@arndb.de>:
> On Thursday 07 February 2013, Geert Uytterhoeven wrote:
>>
>> On Thu, Feb 7, 2013 at 9:01 AM, Michal Simek <monstr@monstr.eu> wrote:
>> > ok. Can you please confirm with me that the same problem is also for
>> > iowrite32be
>> > ioread16be and ioread32be?
>> >
>> > This description seems to me correct for BE and LE.
>> > #define ioread16be(addr)       __be16_to_cpu(__raw_readw(addr))
>> > #define ioread32be(addr)       __be32_to_cpu(__raw_readl(addr))
>> >
>> > #define iowrite16be(v, addr)   __raw_writew(__cpu_to_be16(v), addr)
>> > #define iowrite32be(v, addr)   __raw_writel(__cpu_to_be32(v), addr)
>> >
>> > What do you think?
>>
>> Looks fine to me. Arnd, Ben?
>
> Yes, that would be better. Can you prepare a patch?

Done:
Subject: "asm-generic: io: Fix ioread16/32be and iowrite16/32be"

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Thursday 07 February 2013, Michal Simek wrote:
> Subject: "asm-generic: io: Fix ioread16/32be and iowrite16/32be"

Ok, thanks. If you don't mind, I think this can just go with the other patches
for xsysace that come out of this discussion.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/8 Arnd Bergmann <arnd@arndb.de>:
> On Thursday 07 February 2013, Michal Simek wrote:
>> Subject: "asm-generic: io: Fix ioread16/32be and iowrite16/32be"
>
> Ok, thanks. If you don't mind, I think this can just go with the other patches
> for xsysace that come out of this discussion.

I want to move microblaze to use this asm-generic/io.h that's why I
prefer to send
this with microblaze patch directly to Linus. For me this make more sense that
with sending block device driver changes.

What do you think?

Thanks,
Michal



-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Monday 11 February 2013, Michal Simek wrote:
> 
> 2013/2/8 Arnd Bergmann <arnd@arndb.de>:
> > On Thursday 07 February 2013, Michal Simek wrote:
> >> Subject: "asm-generic: io: Fix ioread16/32be and iowrite16/32be"
> >
> > Ok, thanks. If you don't mind, I think this can just go with the other patches
> > for xsysace that come out of this discussion.
> 
> I want to move microblaze to use this asm-generic/io.h that's why I
> prefer to send
> this with microblaze patch directly to Linus. For me this make more sense that
> with sending block device driver changes.
> 
> What do you think?

Sure, please do it that way. My policy for asm-generic patches is that they
I prefer to have them go through whatever tree needs them, I was just confused
about which one that would be in this case.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/11 Arnd Bergmann <arnd@arndb.de>:
> On Monday 11 February 2013, Michal Simek wrote:
>>
>> 2013/2/8 Arnd Bergmann <arnd@arndb.de>:
>> > On Thursday 07 February 2013, Michal Simek wrote:
>> >> Subject: "asm-generic: io: Fix ioread16/32be and iowrite16/32be"
>> >
>> > Ok, thanks. If you don't mind, I think this can just go with the other patches
>> > for xsysace that come out of this discussion.
>>
>> I want to move microblaze to use this asm-generic/io.h that's why I
>> prefer to send
>> this with microblaze patch directly to Linus. For me this make more sense that
>> with sending block device driver changes.
>>
>> What do you think?
>
> Sure, please do it that way. My policy for asm-generic patches is that they
> I prefer to have them go through whatever tree needs them, I was just confused
> about which one that would be in this case.
>

I have just found that it won't be so easy as I thought because I have found
that microblaze wrong implementation was done because of others device drivers.
It means that I have to fix all device drivers to support big and
little endian accessors
first before I can switch microblaze to asm-generic/io.h. :-(

BTW: If you want to take this patch though your tree then ok, or it
can go through my
microblaze tree. Both ways should just work.

I will send the patch for uarlite when I finish my testing on
microblaze, ppc and arm.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Monday 11 February 2013, Michal Simek wrote:
> I have just found that it won't be so easy as I thought because I have found
> that microblaze wrong implementation was done because of others device drivers.
> It means that I have to fix all device drivers to support big and
> little endian accessors
> first before I can switch microblaze to asm-generic/io.h. :-(
> 
> BTW: If you want to take this patch though your tree then ok, or it
> can go through my
> microblaze tree. Both ways should just work.

Please use your tree then. I don't have any asm-generic patches
lined up for 3.9 myself, so we can save me and Linus a little work
with a trivial pull request that way.
 
> I will send the patch for uarlite when I finish my testing on
> microblaze, ppc and arm.

Ok. Is that the only one that you found to require the "wrong-endian" mode
in microblaze.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/11 Arnd Bergmann <arnd@arndb.de>:
> On Monday 11 February 2013, Michal Simek wrote:
>> I have just found that it won't be so easy as I thought because I have found
>> that microblaze wrong implementation was done because of others device drivers.
>> It means that I have to fix all device drivers to support big and
>> little endian accessors
>> first before I can switch microblaze to asm-generic/io.h. :-(
>>
>> BTW: If you want to take this patch though your tree then ok, or it
>> can go through my
>> microblaze tree. Both ways should just work.
>
> Please use your tree then. I don't have any asm-generic patches
> lined up for 3.9 myself, so we can save me and Linus a little work
> with a trivial pull request that way.
>
>> I will send the patch for uarlite when I finish my testing on
>> microblaze, ppc and arm.
>
> Ok. Is that the only one that you found to require the "wrong-endian" mode
> in microblaze.

Unfortunately no. Another is spi/i2c (sysace as we discuss in this
thread), probably icap
network drivers are ok because they are not shared.
Timer when it is moved to clocksource(not important right now)
xilinx gpio is using __raw IO functions which is incorrect on arm in
connection to barriers.

But it reminds me that maybe the easiest solution is not to use endian
accessors just use two simple macros which should work on all systems.

#define <name>_readreg(offset)		({__raw_readl(offset); rmb(); })
#define <name>_writereg(offset, val)	({wmb(); __raw_writel(val, offset); })

which is probably the faster solution which add minimum additional code
to driver and can also remove endian detection code.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Monday 11 February 2013, Michal Simek wrote:
> Unfortunately no. Another is spi/i2c (sysace as we discuss in this
> thread), probably icap
> network drivers are ok because they are not shared.
> Timer when it is moved to clocksource(not important right now)
> xilinx gpio is using __raw IO functions which is incorrect on arm in
> connection to barriers.
> 
> But it reminds me that maybe the easiest solution is not to use endian
> accessors just use two simple macros which should work on all systems.
> 
> #define <name>_readreg(offset)          ({__raw_readl(offset); rmb(); })
> #define <name>_writereg(offset, val)    ({wmb(); __raw_writel(val, offset); })
> 
> which is probably the faster solution which add minimum additional code
> to driver and can also remove endian detection code.

I tend to disagree. You should never assume that a device is the same
endianess as the the CPU, and you should try not to use the __raw_*
accessors in device drivers either.

In particular, ARM can run both big- and little-endian even though
big-endian is rarely used, so you need to know the endianess for
the device you are talking to rather than assume that it knows
what the CPU does at the time.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Mon, 2013-02-11 at 16:08 +0000, Arnd Bergmann wrote:
> I tend to disagree. You should never assume that a device is the same
> endianess as the the CPU, and you should try not to use the __raw_*
> accessors in device drivers either.
> 
> In particular, ARM can run both big- and little-endian even though
> big-endian is rarely used, so you need to know the endianess for
> the device you are talking to rather than assume that it knows
> what the CPU does at the time.

Part of the problem he might be having is that the way a device is wired
to the bus may be different depending on whether the CPU is running LE
or BE ... or rather should be (in order to preserve byte addresses).

However, not all logic out there might do it right, which means that you
may end up with the wrong wiring if you switch the core around in SW.

It depends how the ARM core operates vs. IO when switched between BE and
LE, does it keep the same lines doing byte 0 or does it keep the MSB/LSB
in the same place (and thus changes which lanes contain byte 0) ?

Depending on how the core does the mode change it may require a wiring
change of IO devices as well... If it changes where byte 0 is, then data
cycles must have the lanes reversed. If it changes where the MSB is,
then address cycles must have the lanes reversed.

I wouldn't be surprised if most SOC bus logic out there gets it wired up
for one and only one case.

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Tuesday 12 February 2013, Benjamin Herrenschmidt wrote:
> It depends how the ARM core operates vs. IO when switched between BE and
> LE, does it keep the same lines doing byte 0 or does it keep the MSB/LSB
> in the same place (and thus changes which lanes contain byte 0) ?

IIRC it changed between older and more recent ARM, at least we have
separate configuration options for "BE-8" big-endian mode on ARMv6
and above, and "BE-32" on older ones.

Currently we don't support platforms upstream that get configured
as BE-32, but the code is there and I know of people using it.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/11 Arnd Bergmann <arnd@arndb.de>:
> On Monday 11 February 2013, Michal Simek wrote:
>> Unfortunately no. Another is spi/i2c (sysace as we discuss in this
>> thread), probably icap
>> network drivers are ok because they are not shared.
>> Timer when it is moved to clocksource(not important right now)
>> xilinx gpio is using __raw IO functions which is incorrect on arm in
>> connection to barriers.
>>
>> But it reminds me that maybe the easiest solution is not to use endian
>> accessors just use two simple macros which should work on all systems.
>>
>> #define <name>_readreg(offset)          ({__raw_readl(offset); rmb(); })
>> #define <name>_writereg(offset, val)    ({wmb(); __raw_writel(val, offset); })
>>
>> which is probably the faster solution which add minimum additional code
>> to driver and can also remove endian detection code.
>
> I tend to disagree. You should never assume that a device is the same
> endianess as the the CPU, and you should try not to use the __raw_*
> accessors in device drivers either.
>
> In particular, ARM can run both big- and little-endian even though
> big-endian is rarely used, so you need to know the endianess for
> the device you are talking to rather than assume that it knows
> what the CPU does at the time.

For high performance IPs using accessors functions is still problematic
because there will be performance regression it means that
from my point of view there still should be any option to "setup"
proper endians for the driver and it can't be setup at run-time.

Sure the question is if drivers like this should be in the mainline.
But if yes, there should be an option to do it in acceptable way.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Tuesday 12 February 2013, Michal Simek wrote:
> > In particular, ARM can run both big- and little-endian even though
> > big-endian is rarely used, so you need to know the endianess for
> > the device you are talking to rather than assume that it knows
> > what the CPU does at the time.
> 
> For high performance IPs using accessors functions is still problematic
> because there will be performance regression it means that
> from my point of view there still should be any option to "setup"
> proper endians for the driver and it can't be setup at run-time.

I did not mean you have to use a run-time detection here, although
that is often the easiest solution. If you know the endianess of
a device for a specific architecture or platform, it is totally
fine to pick that endianess at compile-time and use e.g. the
readl_relaxed() accessors on ARM to give you the lowest access
latency.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/12 Arnd Bergmann <arnd@arndb.de>:
> On Tuesday 12 February 2013, Michal Simek wrote:
>> > In particular, ARM can run both big- and little-endian even though
>> > big-endian is rarely used, so you need to know the endianess for
>> > the device you are talking to rather than assume that it knows
>> > what the CPU does at the time.
>>
>> For high performance IPs using accessors functions is still problematic
>> because there will be performance regression it means that
>> from my point of view there still should be any option to "setup"
>> proper endians for the driver and it can't be setup at run-time.
>
> I did not mean you have to use a run-time detection here, although
> that is often the easiest solution. If you know the endianess of
> a device for a specific architecture or platform, it is totally
> fine to pick that endianess at compile-time and use e.g. the
> readl_relaxed() accessors on ARM to give you the lowest access
> latency.

ok but still there should be well defined how to do it. Let's say
generic Kconfig option.
Or maybe there is also third option to modified code to use proper writes.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Tuesday 12 February 2013, Michal Simek wrote:
> ok but still there should be well defined how to do it. Let's say
> generic Kconfig option.

You cannot solve it in a generic way, since every device has different
needs. In a single SoC, you may have one device that only ever exists
with little-endian I/O, one that is always wired the same way as the
CPU, one that always swaps endianess and one that exists in two
variants but you know which one you have at compile time.

> Or maybe there is also third option to modified code to use proper writes.

You mean run-time code patching? That is seriously wrong here. As Ben
explained, MMIO is slow by definition, so if you add five cycles for
an indirect function call on every MMIO read, that will not even
be noticeable in the 20µs latency that the read has on the bus.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Tue, 2013-02-12 at 11:11 +0100, Michal Simek wrote:
> For high performance IPs using accessors functions is still
> problematic
> because there will be performance regression it means that
> from my point of view there still should be any option to "setup"
> proper endians for the driver and it can't be setup at run-time.
> 
> Sure the question is if drivers like this should be in the mainline.
> But if yes, there should be an option to do it in acceptable way.

No. High performance IP should rely on DMA essentially and be less
affected if not at all. Endianess should affect the control path, not
the data path (unless you got your wiring wrong again :-)

If you want an Kconfig option for an "optimal" version, put the ifdef
trickery in the accessors and make them inline.

We do something like that iirc with OHCI where the type of endianness
needed is a pair of select's and the conditional only happens when both
are set. You can probably do better with feature bits & a mask of
"possible" features relying on the compiler optimisations rather than
the preprocessor.

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Mon, 2013-02-11 at 16:57 +0100, Michal Simek wrote:
> But it reminds me that maybe the easiest solution is not to use endian
> accessors just use two simple macros which should work on all systems.
> 
> #define <name>_readreg(offset)          ({__raw_readl(offset); rmb(); })
> #define <name>_writereg(offset, val)    ({wmb(); __raw_writel(val, offset); })
> 
> which is probably the faster solution which add minimum additional code
> to driver and can also remove endian detection code.

Except that rmb & wmb might not be the right barriers for IOs ...

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/12 Benjamin Herrenschmidt <benh@kernel.crashing.org>:
> On Mon, 2013-02-11 at 16:57 +0100, Michal Simek wrote:
>> But it reminds me that maybe the easiest solution is not to use endian
>> accessors just use two simple macros which should work on all systems.
>>
>> #define <name>_readreg(offset)          ({__raw_readl(offset); rmb(); })
>> #define <name>_writereg(offset, val)    ({wmb(); __raw_writel(val, offset); })
>>
>> which is probably the faster solution which add minimum additional code
>> to driver and can also remove endian detection code.
>
> Except that rmb & wmb might not be the right barriers for IOs ...

Arm is using __iormb in readX macros
#define __iormb()		rmb()
but it is arm specific only that's why I have added there rmb().

Thanks,
Michal




-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Tue, 2013-02-05 at 18:03 +0400, Alexey Brodkin wrote:
> =====
> 
> and its endianess is fixed which is stated in this document 
> (http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf).
> =====
> The Xilinx System ACE Compact Flash chip is a true little-endian device
> =====

So far so good...

> But in its turn Xilinx System ACE Compact Flash chip is attached to 
> CPU's interface bus via some bridge. Depending on interface bus used it 
> could be BVCI-to-MPU (Microprocessor Interface of the System ACE
> Compact Flash solution peripheral) for ARC, PLB for MicroBlaze etc.
> And this bridge in general may do whatever its (HW) developer wants.
> 
> For example for MicroBlaze Xilinx has its XPS Sysace interface 
> controller 
> (http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf) 
> which does bit-swapping from PLB's big-endian bytes to xsysace 
> CF-controller little-endian bytes (though bytes in words are not swapped):
> =====
> The Xilinx System ACE Compact Flash chip is a true little-endian device 
> and the PLB is a big-endian bus. Therefore the XPS System ACE Interface 
> Controller will do a bit-swap in each byte when connecting the PLB data 
> bus to the System ACE data bus as shown in Table 2.

This sounds totally bogus to me. But I'll have a look at the doco to
understand it better. I'll try to do so later today.

> Note however, that the XPS System ACE Interface Controller does not 
> perform the byte swapping necessary to interface to a little-endian 
> device when configured to use 16-bit mode. Therefore, the software 
> drivers provided for this core will perform the necessary byte-swapping 
> to correctly interface to the Xilinx System ACE Compact Flash chip as 
> shown in Table 3.
> =====
> 
> So at this point I'd say that data access should be done differently 
> depending on HW (bridge) used.

Well, bytes accesses should never need any swapping whatsoever. 16-bit
access requires swapping for a LE device for register, never for a data
port. If it does, the bridge is wired incorrectly.

> Another question is do we know for sure that for particular architecture 
> only 1 interface bridge is used. If so then we may select proper 
> accessors per architecture.

There aren't two ways to wire up an interface bridge correctly. I would
advocate not supporting any incorrect wiring in Linux. Doing so would be
going back to supporting horrors like IDE wired backward etc... which we
have mostly gotten rid of.

People need to be educated in this area, and Linux upstream doesn't have
to support any piece of shit anybody comes up with because they can't be
bothered understanding what endianness and byte address invariance mean.

> >>> It is just sharing the same IP across all platforms. Which is better
> >>> than create new devices and new device drivers for it. It means that
> >>> all of them are register compatible but require access with native
> >>> platform endianness as I listed above.
> >>
> >> Every attempt at doing "native platform endianness" has always been a
> >> misguided attempt turning into a trainwreck (see OHCI USB).
> >>
> >> Just pick one endian for the device and stick to it.
> >
> > It is reality and I can't change it. Arnd mentioned it earlier that USB
> >
> >
> >>> It is not a problem to create runtime wrapper and even detect endian
> >>> directly in the driver
> >>> but the point if this is the proper design.
> >>> Also ioread32 and ioread32be shouldn't be used on ARM because there
> >>> are missing memory barriers.
> >>
> >> Then fix them, they shouldn't be, it's a bug, it will break many other
> >> drivers. They should be fully equivalent to readl.
> >
> > I want to be sure about this. I have parsed this again with closer look and
> > seems to me that ioread32 is equal to readl and iowrite32 to writel.
> > Arnd: Am I right?
> >
> > Thanks,
> > Michal
> 
> -Alexey
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/06/2013 01:00 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2013-02-05 at 18:03 +0400, Alexey Brodkin wrote:
>> =====
>>
>> and its endianess is fixed which is stated in this document
>> (http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf).
>> =====
>> The Xilinx System ACE Compact Flash chip is a true little-endian device
>> =====
>
> So far so good...
>
>> But in its turn Xilinx System ACE Compact Flash chip is attached to
>> CPU's interface bus via some bridge. Depending on interface bus used it
>> could be BVCI-to-MPU (Microprocessor Interface of the System ACE
>> Compact Flash solution peripheral) for ARC, PLB for MicroBlaze etc.
>> And this bridge in general may do whatever its (HW) developer wants.
>>
>> For example for MicroBlaze Xilinx has its XPS Sysace interface
>> controller
>> (http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf)
>> which does bit-swapping from PLB's big-endian bytes to xsysace
>> CF-controller little-endian bytes (though bytes in words are not swapped):
>> =====
>> The Xilinx System ACE Compact Flash chip is a true little-endian device
>> and the PLB is a big-endian bus. Therefore the XPS System ACE Interface
>> Controller will do a bit-swap in each byte when connecting the PLB data
>> bus to the System ACE data bus as shown in Table 2.
>
> This sounds totally bogus to me. But I'll have a look at the doco to
> understand it better. I'll try to do so later today.
>
>> Note however, that the XPS System ACE Interface Controller does not
>> perform the byte swapping necessary to interface to a little-endian
>> device when configured to use 16-bit mode. Therefore, the software
>> drivers provided for this core will perform the necessary byte-swapping
>> to correctly interface to the Xilinx System ACE Compact Flash chip as
>> shown in Table 3.
>> =====
>>
>> So at this point I'd say that data access should be done differently
>> depending on HW (bridge) used.
>
> Well, bytes accesses should never need any swapping whatsoever. 16-bit
> access requires swapping for a LE device for register, never for a data
> port. If it does, the bridge is wired incorrectly.

Sorry, saying "data access" here I meant accessing 16-bit registers 
indeed. Because most of configuration/control is done via setting values 
in registers. I should have selected proper terms.

>> Another question is do we know for sure that for particular architecture
>> only 1 interface bridge is used. If so then we may select proper
>> accessors per architecture.
>
> There aren't two ways to wire up an interface bridge correctly. I would
> advocate not supporting any incorrect wiring in Linux. Doing so would be
> going back to supporting horrors like IDE wired backward etc... which we
> have mostly gotten rid of.
>
> People need to be educated in this area, and Linux upstream doesn't have
> to support any piece of shit anybody comes up with because they can't be
> bothered understanding what endianness and byte address invariance mean.

Sounds good but how should one tell which approach is correct? For 
example here - is the one implemented by Xilinx is golden reference or not?

>>>>> It is just sharing the same IP across all platforms. Which is better
>>>>> than create new devices and new device drivers for it. It means that
>>>>> all of them are register compatible but require access with native
>>>>> platform endianness as I listed above.
>>>>
>>>> Every attempt at doing "native platform endianness" has always been a
>>>> misguided attempt turning into a trainwreck (see OHCI USB).
>>>>
>>>> Just pick one endian for the device and stick to it.
>>>
>>> It is reality and I can't change it. Arnd mentioned it earlier that USB
>>>
>>>
>>>>> It is not a problem to create runtime wrapper and even detect endian
>>>>> directly in the driver
>>>>> but the point if this is the proper design.
>>>>> Also ioread32 and ioread32be shouldn't be used on ARM because there
>>>>> are missing memory barriers.
>>>>
>>>> Then fix them, they shouldn't be, it's a bug, it will break many other
>>>> drivers. They should be fully equivalent to readl.
>>>
>>> I want to be sure about this. I have parsed this again with closer look and
>>> seems to me that ioread32 is equal to readl and iowrite32 to writel.
>>> Arnd: Am I right?
>>>
>>> Thanks,
>>> Michal
>>
>> -Alexey
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
>
-Alexey

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Wed, 2013-02-06 at 01:25 +0400, Alexey Brodkin wrote:
> Sounds good but how should one tell which approach is correct? For 
> example here - is the one implemented by Xilinx is golden reference or
> not?

So I'm reading that PDF you pointed to. So far what I can see is:

 - In 8-bit mode you only do 8-bit accesses, so endianness should be
totally irrelevant (at least the pdf says so)

 - In 16-bit mode, that's where things become interesting... the doc
says:

  PLB Data Bus 		| System ACE Data Bus
  ----------------------+--------------------
  PLB_DBus[8 : 15]	| SysACE_MPD[15 : 8]
  PLB_DBus[0 : 7] 	| SysACE_MPD[7 : 0]

Now, I'm not 100% of the bit numbering used by Xilinx here but it smells
like PLB used ppc numbering and SystemACE use the classic numbering, in
which case the above would mean that the MSB of the PLB is connected to
the LSB of the SystemACE and vice-versa.

If that is the case then this is the *correct* wiring and means that the
data port (if any) doesn't need any byteswapping. It also means that the
registers need byteswap on BE, as expected for a LE device. IE. Just
always use ioread32 (or _rep variants for the data port if there's such
a thing on it).

So that looks good... unless I misunderstood the Xilinx spec, this looks
like the right way to do and the only one we should support.

The various other mentions in the text of "bit swapping" vs "byte swapping"
and reference to SW swapping are all just very confused which makes me
think that while the wiring up ended up being correct, this is in part
by accident (or whoever wrote the doc didn't understand what this is
all about).

So is there a known implementation that gets it backward ? Unless it's
a very popular and very useful one to support I would advocate just
not doing so.

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Tue, Feb 5, 2013 at 11:07 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2013-02-06 at 01:25 +0400, Alexey Brodkin wrote:
>> Sounds good but how should one tell which approach is correct? For
>> example here - is the one implemented by Xilinx is golden reference or
>> not?
>
> So I'm reading that PDF you pointed to. So far what I can see is:
>
>  - In 8-bit mode you only do 8-bit accesses, so endianness should be
> totally irrelevant (at least the pdf says so)
>
>  - In 16-bit mode, that's where things become interesting... the doc
> says:
>
>   PLB Data Bus          | System ACE Data Bus
>   ----------------------+--------------------
>   PLB_DBus[8 : 15]      | SysACE_MPD[15 : 8]
>   PLB_DBus[0 : 7]       | SysACE_MPD[7 : 0]
>
> Now, I'm not 100% of the bit numbering used by Xilinx here but it smells
> like PLB used ppc numbering and SystemACE use the classic numbering, in
> which case the above would mean that the MSB of the PLB is connected to
> the LSB of the SystemACE and vice-versa.
>
> If that is the case then this is the *correct* wiring and means that the
> data port (if any) doesn't need any byteswapping. It also means that the
> registers need byteswap on BE, as expected for a LE device. IE. Just
> always use ioread32 (or _rep variants for the data port if there's such
> a thing on it).
>
> So that looks good... unless I misunderstood the Xilinx spec, this looks
> like the right way to do and the only one we should support.

Huh? That makes no sense. This device out in the wild with both big
and little endian bus attachments. You can argue all day that one of
them is wrong, but it doesn't matter. It exists, is used, and must be
supported. In fact, the driver already knows about this and figures
out at runtime how the device is wired up to the bus. This is not the
problem.

BTW, that document describes only one of the systemace bus
attachments. There is a different on used on Microblaze little-endian,
and some boards have the SystemACE directly wired to the SoC external
bus (no adapter IP).

The only problem that I see is that the ARM and Microblaze
ioread16be/iowrite16be helpers are missing barriers which smells like
a bug and should be fixed.

Michal, have you tested Alexey's patch? If it works for you then I'm
comfortable with acking it. It looks correct to me.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Wednesday 06 February 2013, Grant Likely wrote:
> The only problem that I see is that the ARM and Microblaze
> ioread16be/iowrite16be helpers are missing barriers which smells like
> a bug and should be fixed.

It looks correct to me on ARM, we use the same barriers in ioread and
iowrite that we use in readl/writel.

Microblaze uses no barriers in any of the I/O accessor families
(readl, ioread32, in_le32, inl, inl_p), so it presumably doesn't
require any because it is fully ordered, at least changing from
in_le32 to ioread32 won't make it worse.
On a related note, the inb/outb style accessors are bogus on microblaze
and should be removed, but that is a different discussion. I should
have updated my old patches to introduce a meaningful CONFIG_NO_IOPORT
ages ago, and without them it probably breaks.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Wed, 2013-02-06 at 10:14 +0000, Grant Likely wrote:
> 
> Huh? That makes no sense. This device out in the wild with both big
> and little endian bus attachments. You can argue all day that one of
> them is wrong, but it doesn't matter. It exists, is used, and must be
> supported.

No. That's where you are VERY wrong. We don't have to support crap and
arguably shouldn't if that can give an incentive to vendors to fix their
stuff. If you don't believe me, ask Linus :-)

>  In fact, the driver already knows about this and figures
> out at runtime how the device is wired up to the bus. This is not the
> problem.

Except that this is very gross, especially when you observe that in the
busted "big endian" case, it has to byteswap the bloody data port.

So you end up having to do that gross hack with separate accessors for
registers vs. data and not able to use the _rep variants, which also
means that on platforms like ppc, you end up with a memory barrier on
every access (or more), which is going to slow things down enormously.

> BTW, that document describes only one of the systemace bus
> attachments. There is a different on used on Microblaze little-endian,
> and some boards have the SystemACE directly wired to the SoC external
> bus (no adapter IP).
> 
> The only problem that I see is that the ARM and Microblaze
> ioread16be/iowrite16be helpers are missing barriers which smells like
> a bug and should be fixed.
> 
> Michal, have you tested Alexey's patch? If it works for you then I'm
> comfortable with acking it. It looks correct to me.

No, the real problem is that the "big endian" wiring is totally busted
and the HW guys who came with it must be taught a lesson. Not supporting
that crap might be one way to do it.

Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/07/2013 01:35 AM, Benjamin Herrenschmidt wrote:
> On Wed, 2013-02-06 at 10:14 +0000, Grant Likely wrote:
>>
>> Huh? That makes no sense. This device out in the wild with both big
>> and little endian bus attachments. You can argue all day that one of
>> them is wrong, but it doesn't matter. It exists, is used, and must be
>> supported.
>
> No. That's where you are VERY wrong. We don't have to support crap and
> arguably shouldn't if that can give an incentive to vendors to fix their
> stuff. If you don't believe me, ask Linus :-)
>
>>   In fact, the driver already knows about this and figures
>> out at runtime how the device is wired up to the bus. This is not the
>> problem.
>
> Except that this is very gross, especially when you observe that in the
> busted "big endian" case, it has to byteswap the bloody data port.
>
> So you end up having to do that gross hack with separate accessors for
> registers vs. data and not able to use the _rep variants, which also
> means that on platforms like ppc, you end up with a memory barrier on
> every access (or more), which is going to slow things down enormously.

BTW I've just realized that in case if there's no bridge between CPU and 
CF-controller or if this bridge is "transparent" (does no swapping
neither bytes nor bits) our data accessors here should be changed.

Isn't it strange in "ace_datain_le16" use "ioread16be" or the one it was 
here initially "in_be16"?
With BE ones I'd say similar changes should be done.

So finally I see them implemented this way:
===============
/* BE part */
static void ace_datain_be16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *dst = ace->data_ptr;
	while (i--)
		*dst++ = ioread16be(ace->baseaddr + 0x40);
	ace->data_ptr = dst;
}

static void ace_dataout_be16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *src = ace->data_ptr;
	while (i--)
		iowrite16be(*src++, ace->baseaddr + 0x40);
	ace->data_ptr = src;
}

/* LE part*/
static void ace_datain_le16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *dst = ace->data_ptr;
	while (i--)
		*dst++ = ioread16(ace->baseaddr + 0x40);
	ace->data_ptr = dst;
}

static void ace_dataout_le16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *src = ace->data_ptr;
	while (i--)
		iowrite16(*src++, ace->baseaddr + 0x40);
	ace->data_ptr = src;
}
===============

Correct me if I'm wrong here.

And at least these accessors for LE got xsysace perfectly working on our 
FPGA platform (little-endian ARC700 on Xilinx ml-509 with our own 
BVCI-to-MPU bridge that does no swapping).

I have to confess that I didn't properly tested initial patch on real HW 
- it was only sort of cosmetic clean-up.

-Alexey

>> BTW, that document describes only one of the systemace bus
>> attachments. There is a different on used on Microblaze little-endian,
>> and some boards have the SystemACE directly wired to the SoC external
>> bus (no adapter IP).
>>
>> The only problem that I see is that the ARM and Microblaze
>> ioread16be/iowrite16be helpers are missing barriers which smells like
>> a bug and should be fixed.
>>
>> Michal, have you tested Alexey's patch? If it works for you then I'm
>> comfortable with acking it. It looks correct to me.
>
> No, the real problem is that the "big endian" wiring is totally busted
> and the HW guys who came with it must be taught a lesson. Not supporting
> that crap might be one way to do it.
>
> Ben.
>
>

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Thu, 2013-02-07 at 16:09 +0400, Alexey Brodkin wrote:
> 
> BTW I've just realized that in case if there's no bridge between CPU and 
> CF-controller or if this bridge is "transparent" (does no swapping
> neither bytes nor bits) our data accessors here should be changed.
> 
> Isn't it strange in "ace_datain_le16" use "ioread16be" or the one it was 
> here initially "in_be16"?
> With BE ones I'd say similar changes should be done.

This is part of the problem with those bloody attempts at dealing with
broken shit, they get the "right" case wrong :-)

So yes, if the bridge is wired up properly:

 - Register access is LE

 - Data port access is native endian (always) because what matters here
is not endianness but byte ordering (ie, which byte is 0) and if things
are wired properly, this is preserved.

So the correct things is in that case is to use ioread16_rep which will
do the right thing (and avoid the extra barriers and C loop) for the
data in/out code, and ioread16/iowrite16 for the registers.

For the "swapped" case, I would suggest using ioread16be for the registers
for the data port, use ioread16_rep followed by a pass of byteswap. I assume
that this incorrect wiring case only happens on BE platforms right ?

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Thu, 2013-02-07 at 23:20 +1100, Benjamin Herrenschmidt wrote:

> For the "swapped" case, I would suggest using ioread16be for the registers
> for the data port, use ioread16_rep followed by a pass of byteswap. I assume
> that this incorrect wiring case only happens on BE platforms right ?

Of course that won't work on writes unless you use a bounce buffer, but
heh, who cares, it's busted anyway ?

Another option is to stick with a loop of ioread16be/iowrite16be, it
*should* work, but it will have too many memory barriers, the impact
will vary depending on the core.

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/7 Alexey Brodkin <Alexey.Brodkin@synopsys.com>:
> On 02/07/2013 01:35 AM, Benjamin Herrenschmidt wrote:
>>
>> On Wed, 2013-02-06 at 10:14 +0000, Grant Likely wrote:
>>>
>>>
>>> Huh? That makes no sense. This device out in the wild with both big
>>> and little endian bus attachments. You can argue all day that one of
>>> them is wrong, but it doesn't matter. It exists, is used, and must be
>>> supported.
>>
>>
>> No. That's where you are VERY wrong. We don't have to support crap and
>> arguably shouldn't if that can give an incentive to vendors to fix their
>> stuff. If you don't believe me, ask Linus :-)
>>
>>>   In fact, the driver already knows about this and figures
>>> out at runtime how the device is wired up to the bus. This is not the
>>> problem.
>>
>>
>> Except that this is very gross, especially when you observe that in the
>> busted "big endian" case, it has to byteswap the bloody data port.
>>
>> So you end up having to do that gross hack with separate accessors for
>> registers vs. data and not able to use the _rep variants, which also
>> means that on platforms like ppc, you end up with a memory barrier on
>> every access (or more), which is going to slow things down enormously.
>
>
> BTW I've just realized that in case if there's no bridge between CPU and
> CF-controller or if this bridge is "transparent" (does no swapping
> neither bytes nor bits) our data accessors here should be changed.
>
> Isn't it strange in "ace_datain_le16" use "ioread16be" or the one it was
> here initially "in_be16"?
> With BE ones I'd say similar changes should be done.
>
> So finally I see them implemented this way:
> ===============
> /* BE part */
>
> static void ace_datain_be16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *dst = ace->data_ptr;
>         while (i--)
>                 *dst++ = ioread16be(ace->baseaddr + 0x40);
>         ace->data_ptr = dst;
> }
>
> static void ace_dataout_be16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *src = ace->data_ptr;
>         while (i--)
>                 iowrite16be(*src++, ace->baseaddr + 0x40);
>         ace->data_ptr = src;
> }
>
> /* LE part*/
>
> static void ace_datain_le16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *dst = ace->data_ptr;
>         while (i--)
>                 *dst++ = ioread16(ace->baseaddr + 0x40);
>         ace->data_ptr = dst;
> }
>
> static void ace_dataout_le16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *src = ace->data_ptr;
>         while (i--)
>                 iowrite16(*src++, ace->baseaddr + 0x40);
>         ace->data_ptr = src;
> }
> ===============
>
> Correct me if I'm wrong here.
>
> And at least these accessors for LE got xsysace perfectly working on our
> FPGA platform (little-endian ARC700 on Xilinx ml-509 with our own
> BVCI-to-MPU bridge that does no swapping).
>
> I have to confess that I didn't properly tested initial patch on real HW -
> it was only sort of cosmetic clean-up.

The origin patch (after some microblaze ioread/iowrite fixes) works ok,
These additional changes is breaking sysace on microblaze big endian
ml505 16bit mode.
8bit mode just works

Also I have looked at our tree and I see that the mainline kernel misses
one patch which was sent by Graeme Smecher and it was also Acked-by Grant.
It is called "Fix device name assignment for SystemACE (from "xs`" to "xsa")."
Let me resend it too.

Alexey: Can you please confirm that on your arc platform you see broken CF
partitions name?

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/07/2013 06:31 PM, Michal Simek wrote:
> The origin patch (after some microblaze ioread/iowrite fixes) works ok,
> These additional changes is breaking sysace on microblaze big endian
> ml505 16bit mode.
> 8bit mode just works
>
> Also I have looked at our tree and I see that the mainline kernel misses
> one patch which was sent by Graeme Smecher and it was also Acked-by Grant.
> It is called "Fix device name assignment for SystemACE (from "xs`" to "xsa")."
> Let me resend it too.
>
> Alexey: Can you please confirm that on your arc platform you see broken CF
> partitions name?
>
> Thanks,
> Michal
>

Indeed I had it and fixed myself)
I didn't know about this fix so I've just sent another fix for it - 
https://patchwork.kernel.org/patch/2110811/

Anyways good to know now it is fixed.

Thanks,
Alexey

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Wed, Feb 6, 2013 at 9:35 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2013-02-06 at 10:14 +0000, Grant Likely wrote:
>>
>> Huh? That makes no sense. This device out in the wild with both big
>> and little endian bus attachments. You can argue all day that one of
>> them is wrong, but it doesn't matter. It exists, is used, and must be
>> supported.
>
> No. That's where you are VERY wrong. We don't have to support crap and
> arguably shouldn't if that can give an incentive to vendors to fix their
> stuff. If you don't believe me, ask Linus :-)

As far as horrible hardware goes, this device comes no where close to
some of the stuff I've worked on. The driver is self contained. It
doesn't have any nasty hooks into the rest of the kernel and most
importantly it currently *works* and is used.

>>  In fact, the driver already knows about this and figures
>> out at runtime how the device is wired up to the bus. This is not the
>> problem.
>
> Except that this is very gross, especially when you observe that in the
> busted "big endian" case, it has to byteswap the bloody data port.
>
> So you end up having to do that gross hack with separate accessors for
> registers vs. data and not able to use the _rep variants, which also
> means that on platforms like ppc, you end up with a memory barrier on
> every access (or more), which is going to slow things down enormously.

I don't see why the _rep variants aren't usable here. The only reason
I didn't use them when I wrote the driver in the first place was I was
a n00b kernel hacker and I didn't know they were there.

g.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Thu, Feb 7, 2013 at 2:39 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 6, 2013 at 9:35 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>>>  In fact, the driver already knows about this and figures
>>> out at runtime how the device is wired up to the bus. This is not the
>>> problem.
>>
>> Except that this is very gross, especially when you observe that in the
>> busted "big endian" case, it has to byteswap the bloody data port.
>>
>> So you end up having to do that gross hack with separate accessors for
>> registers vs. data and not able to use the _rep variants, which also
>> means that on platforms like ppc, you end up with a memory barrier on
>> every access (or more), which is going to slow things down enormously.
>
> I don't see why the _rep variants aren't usable here. The only reason
> I didn't use them when I wrote the driver in the first place was I was
> a n00b kernel hacker and I didn't know they were there.

The 8-bit variant is different though because the hardware requires
pingponging between odd and even byte addresses to flush the fifo.
Reading a data port even address (0x40) gives the least significant
byte. Reading from an odd address (0x41) give the MSB and pops the
data off the FIFO. So, yes, the _rep variant can't be used in 8-bit
mode. It should still be fine in 16-bit.

page 45: http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/07/2013 06:51 PM, Grant Likely wrote:
> On Thu, Feb 7, 2013 at 2:39 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Feb 6, 2013 at 9:35 PM, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>>>>   In fact, the driver already knows about this and figures
>>>> out at runtime how the device is wired up to the bus. This is not the
>>>> problem.
>>>
>>> Except that this is very gross, especially when you observe that in the
>>> busted "big endian" case, it has to byteswap the bloody data port.
>>>
>>> So you end up having to do that gross hack with separate accessors for
>>> registers vs. data and not able to use the _rep variants, which also
>>> means that on platforms like ppc, you end up with a memory barrier on
>>> every access (or more), which is going to slow things down enormously.
>>
>> I don't see why the _rep variants aren't usable here. The only reason
>> I didn't use them when I wrote the driver in the first place was I was
>> a n00b kernel hacker and I didn't know they were there.
>
> The 8-bit variant is different though because the hardware requires
> pingponging between odd and even byte addresses to flush the fifo.
> Reading a data port even address (0x40) gives the least significant
> byte. Reading from an odd address (0x41) give the MSB and pops the
> data off the FIFO. So, yes, the _rep variant can't be used in 8-bit
> mode. It should still be fine in 16-bit.
>
> page 45: http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf
>

Ok, so may I do a re-spin with these changes:
1. In "ace_in_be16" use "ioread16be"
2. In "ace_out_be16" use "iowrite16be"
3. In "ace_in_le16" use "ioread16"
4. In "ace_out_le16" use "iowrite16"
5. In "ace_datain_le16" use "ioread16_rep"
6. In "ace_dataout_le16" use "iowrite16_rep"

not sure about items for "ace_datain/out_be16" - what about _rep options 
here?

-Alexey

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Thu, Feb 7, 2013 at 3:12 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> On 02/07/2013 06:51 PM, Grant Likely wrote:
>>
>> On Thu, Feb 7, 2013 at 2:39 PM, Grant Likely <grant.likely@secretlab.ca>
>> wrote:
>>>
>>> On Wed, Feb 6, 2013 at 9:35 PM, Benjamin Herrenschmidt
>>> <benh@kernel.crashing.org> wrote:
>>>>>
>>>>>   In fact, the driver already knows about this and figures
>>>>> out at runtime how the device is wired up to the bus. This is not the
>>>>> problem.
>>>>
>>>>
>>>> Except that this is very gross, especially when you observe that in the
>>>> busted "big endian" case, it has to byteswap the bloody data port.
>>>>
>>>> So you end up having to do that gross hack with separate accessors for
>>>> registers vs. data and not able to use the _rep variants, which also
>>>> means that on platforms like ppc, you end up with a memory barrier on
>>>> every access (or more), which is going to slow things down enormously.
>>>
>>>
>>> I don't see why the _rep variants aren't usable here. The only reason
>>> I didn't use them when I wrote the driver in the first place was I was
>>> a n00b kernel hacker and I didn't know they were there.
>>
>>
>> The 8-bit variant is different though because the hardware requires
>> pingponging between odd and even byte addresses to flush the fifo.
>> Reading a data port even address (0x40) gives the least significant
>> byte. Reading from an odd address (0x41) give the MSB and pops the
>> data off the FIFO. So, yes, the _rep variant can't be used in 8-bit
>> mode. It should still be fine in 16-bit.
>>
>> page 45: http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf
>>
>
> Ok, so may I do a re-spin with these changes:

There are two things here. 1) changing the accessors used. 2)
switching the endianess as a bug fix. Any changes to the endian access
should be a separate patch which a description of what is needed.

> 1. In "ace_in_be16" use "ioread16be"
> 2. In "ace_out_be16" use "iowrite16be"
> 3. In "ace_in_le16" use "ioread16"
> 4. In "ace_out_le16" use "iowrite16"

Yes

> 5. In "ace_datain_le16" use "ioread16_rep"
> 6. In "ace_dataout_le16" use "iowrite16_rep"

Maybe. In a separate patch. Hmmm... I guess there isn't an
ioread16be_rep variant. Oh well. Check first with Michal on LE
microblaze before making the change. If it doesn't work for him the
more understanding is needed. I was pretty sure the LE variant already
worked.

> not sure about items for "ace_datain/out_be16" - what about _rep options
> here?

ioread16_rep should be fine. The ace_data{in,out}_be16 routines need
to use the LE accessor. The existing code is definitely correct in
this respect.

g.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/07/2013 07:23 PM, Grant Likely wrote:
> On Thu, Feb 7, 2013 at 3:12 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
>> On 02/07/2013 06:51 PM, Grant Likely wrote:
>>>
>>> On Thu, Feb 7, 2013 at 2:39 PM, Grant Likely <grant.likely@secretlab.ca>
>>> wrote:
>>>>
>>>> On Wed, Feb 6, 2013 at 9:35 PM, Benjamin Herrenschmidt
>>>> <benh@kernel.crashing.org> wrote:
>>>>>>
>>>>>>    In fact, the driver already knows about this and figures
>>>>>> out at runtime how the device is wired up to the bus. This is not the
>>>>>> problem.
>>>>>
>>>>>
>>>>> Except that this is very gross, especially when you observe that in the
>>>>> busted "big endian" case, it has to byteswap the bloody data port.
>>>>>
>>>>> So you end up having to do that gross hack with separate accessors for
>>>>> registers vs. data and not able to use the _rep variants, which also
>>>>> means that on platforms like ppc, you end up with a memory barrier on
>>>>> every access (or more), which is going to slow things down enormously.
>>>>
>>>>
>>>> I don't see why the _rep variants aren't usable here. The only reason
>>>> I didn't use them when I wrote the driver in the first place was I was
>>>> a n00b kernel hacker and I didn't know they were there.
>>>
>>>
>>> The 8-bit variant is different though because the hardware requires
>>> pingponging between odd and even byte addresses to flush the fifo.
>>> Reading a data port even address (0x40) gives the least significant
>>> byte. Reading from an odd address (0x41) give the MSB and pops the
>>> data off the FIFO. So, yes, the _rep variant can't be used in 8-bit
>>> mode. It should still be fine in 16-bit.
>>>
>>> page 45: http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf
>>>
>>
>> Ok, so may I do a re-spin with these changes:
>
> There are two things here. 1) changing the accessors used. 2)
> switching the endianess as a bug fix. Any changes to the endian access
> should be a separate patch which a description of what is needed.
>
>> 1. In "ace_in_be16" use "ioread16be"
>> 2. In "ace_out_be16" use "iowrite16be"
>> 3. In "ace_in_le16" use "ioread16"
>> 4. In "ace_out_le16" use "iowrite16"
>
> Yes
>
>> 5. In "ace_datain_le16" use "ioread16_rep"
>> 6. In "ace_dataout_le16" use "iowrite16_rep"
>
> Maybe. In a separate patch. Hmmm... I guess there isn't an
> ioread16be_rep variant. Oh well. Check first with Michal on LE
> microblaze before making the change. If it doesn't work for him the
> more understanding is needed. I was pretty sure the LE variant already
> worked.
>
>> not sure about items for "ace_datain/out_be16" - what about _rep options
>> here?
>
> ioread16_rep should be fine. The ace_data{in,out}_be16 routines need
> to use the LE accessor. The existing code is definitely correct in
> this respect.

Well, if "ioread16_rep" is used in both "ace_data{in,out}_be16" and 
"ace_data{in,out}_le16" then what is a difference between them?
Whether there's a subtle difference still exists and I cannot see it or 
unified accessor could be used from now on at least for data access.

What do you think?

-Alexey

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Thu, Feb 7, 2013 at 3:28 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> On 02/07/2013 07:23 PM, Grant Likely wrote:
>>
>> On Thu, Feb 7, 2013 at 3:12 PM, Alexey Brodkin
>> <Alexey.Brodkin@synopsys.com> wrote:
>>>
>>> On 02/07/2013 06:51 PM, Grant Likely wrote:
>>>>
>>>>
>>>> On Thu, Feb 7, 2013 at 2:39 PM, Grant Likely <grant.likely@secretlab.ca>
>>>> wrote:
>>>>>
>>>>>
>>>>> On Wed, Feb 6, 2013 at 9:35 PM, Benjamin Herrenschmidt
>>>>> <benh@kernel.crashing.org> wrote:
>>>>>>>
>>>>>>>
>>>>>>>    In fact, the driver already knows about this and figures
>>>>>>> out at runtime how the device is wired up to the bus. This is not the
>>>>>>> problem.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Except that this is very gross, especially when you observe that in
>>>>>> the
>>>>>> busted "big endian" case, it has to byteswap the bloody data port.
>>>>>>
>>>>>> So you end up having to do that gross hack with separate accessors for
>>>>>> registers vs. data and not able to use the _rep variants, which also
>>>>>> means that on platforms like ppc, you end up with a memory barrier on
>>>>>> every access (or more), which is going to slow things down enormously.
>>>>>
>>>>>
>>>>>
>>>>> I don't see why the _rep variants aren't usable here. The only reason
>>>>> I didn't use them when I wrote the driver in the first place was I was
>>>>> a n00b kernel hacker and I didn't know they were there.
>>>>
>>>>
>>>>
>>>> The 8-bit variant is different though because the hardware requires
>>>> pingponging between odd and even byte addresses to flush the fifo.
>>>> Reading a data port even address (0x40) gives the least significant
>>>> byte. Reading from an odd address (0x41) give the MSB and pops the
>>>> data off the FIFO. So, yes, the _rep variant can't be used in 8-bit
>>>> mode. It should still be fine in 16-bit.
>>>>
>>>> page 45:
>>>> http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf
>>>>
>>>
>>> Ok, so may I do a re-spin with these changes:
>>
>>
>> There are two things here. 1) changing the accessors used. 2)
>> switching the endianess as a bug fix. Any changes to the endian access
>> should be a separate patch which a description of what is needed.
>>
>>> 1. In "ace_in_be16" use "ioread16be"
>>> 2. In "ace_out_be16" use "iowrite16be"
>>> 3. In "ace_in_le16" use "ioread16"
>>> 4. In "ace_out_le16" use "iowrite16"
>>
>>
>> Yes
>>
>>> 5. In "ace_datain_le16" use "ioread16_rep"
>>> 6. In "ace_dataout_le16" use "iowrite16_rep"
>>
>>
>> Maybe. In a separate patch. Hmmm... I guess there isn't an
>> ioread16be_rep variant. Oh well. Check first with Michal on LE
>> microblaze before making the change. If it doesn't work for him the
>> more understanding is needed. I was pretty sure the LE variant already
>> worked.
>>
>>> not sure about items for "ace_datain/out_be16" - what about _rep options
>>> here?
>>
>>
>> ioread16_rep should be fine. The ace_data{in,out}_be16 routines need
>> to use the LE accessor. The existing code is definitely correct in
>> this respect.
>
>
> Well, if "ioread16_rep" is used in both "ace_data{in,out}_be16" and
> "ace_data{in,out}_le16" then what is a difference between them?
> Whether there's a subtle difference still exists and I cannot see it or
> unified accessor could be used from now on at least for data access.

I've just spent some quality time with a piece of paper, and I think
I've figured it out...

Starting with register (non-data) access. The bus bindings are such
that on both BE and LE systems a native 16 bit read results in the
bits being in the correct order. On powerpc, you want to do a BE read
because the device appears as a BE device, and on LE systems a LE read
is wanted because that is how the device is wired. So far this makes
sense and matches the driver.

The same is true for the data port. A BE read does the right thing on
a BE platform, and LE read on a LE platform. (again, this is all about
bus attachment). However, the difference is what is then done with the
data.

For register reads, the driver uses the value directly. Either by
writing a 16 bit value to a register or reading and interpreting a 16
bit value. In both case the drive is directly interested in the 16bit
word

However, for the data port, the data is streamed out of the FIFO and
stored in memory for the block subsystem to use. A read from the data
port (assuming 'native' accesses are used as described above) obtains
16bits of data from the disk. Disk offsets are byte oriented, but
since it they are 16 bit reads, two bytes are read at a time:
First read:  offset 0 in LSB, offset 1 MSB
Second read: offset 2 in LSB, offset 3 in MSB
Third read: offset 4 in LSB, offset 5 in MSB

etc.

Then that data needs to be stored into memory. This is where things
are different with native 16 bit stores:
  On a BE memory system. MSB in byte address 0 and LSB in byte address 1.
  On a LE memory system. MSB in byte address 1 and LSB in byte address 0.

The block subsystem deals with byte oriented buffers; so given the way
data is arranged in the data port we want to always make sure the LSB
goes into address 0. The cause of problems isn't the BE vs LE bus
attachment. It is the different memory system orientation. Using
ioread16/iowrite16 has the right behaviour, but it's kind of a
backwards way to go about it.... It isn't that we want a be16_to_cpu()
or le16_to_cpu() on the data port read, but rather a cpu_to_le16() on
the store to memory regardless of the endianess of the platform.

So, if I'm correct that means that for the data port (specifically
copying between RAM and the data port) using ioread16/iowrite16 on the
data port results in the correct behaviour. It also means that LE
support in the current driver is broken.

The test will be whether or not your change works for LE microblaze
which I'm hoping Michal is going to take for a spin.

The ironic thing is that if the BE PPC/MB hardware engineers hadn't
swapped the bytelanes then we would *still* have to do special thinks
in the hardware; except it would be the data port accesses that would
need extra work, instead of the other registers.

g.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/07/2013 08:44 PM, Grant Likely wrote:
>
> Then that data needs to be stored into memory. This is where things
> are different with native 16 bit stores:
>    On a BE memory system. MSB in byte address 0 and LSB in byte address 1.
>    On a LE memory system. MSB in byte address 1 and LSB in byte address 0.
>
> The block subsystem deals with byte oriented buffers; so given the way
> data is arranged in the data port we want to always make sure the LSB
> goes into address 0. The cause of problems isn't the BE vs LE bus
> attachment. It is the different memory system orientation. Using
> ioread16/iowrite16 has the right behaviour, but it's kind of a
> backwards way to go about it.... It isn't that we want a be16_to_cpu()
> or le16_to_cpu() on the data port read, but rather a cpu_to_le16() on
> the store to memory regardless of the endianess of the platform.
>
> So, if I'm correct that means that for the data port (specifically
> copying between RAM and the data port) using ioread16/iowrite16 on the
> data port results in the correct behaviour. It also means that LE
> support in the current driver is broken.

That matches my earlier note when I wrote that for correct work on LE 
(note I'm on ARC, not PPC/MB) I needed to use "io{read|write}16" in 
"ace_data{in|out}_le16".

With original "io{read|write}16be" instead data was corrupted.

-Alexey

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Thu, Feb 7, 2013 at 4:56 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> On 02/07/2013 08:44 PM, Grant Likely wrote:
>> So, if I'm correct that means that for the data port (specifically
>> copying between RAM and the data port) using ioread16/iowrite16 on the
>> data port results in the correct behaviour. It also means that LE
>> support in the current driver is broken.
>
> That matches my earlier note when I wrote that for correct work on LE (note
> I'm on ARC, not PPC/MB) I needed to use "io{read|write}16" in
> "ace_data{in|out}_le16".
>
> With original "io{read|write}16be" instead data was corrupted.

In which case your bug-fix patch should drop the
ace_datain_le16/ace_dataout_le16 variants entirely and use the be16
ones for both (renaming appropriately).

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/07/2013 09:16 PM, Grant Likely wrote:
> On Thu, Feb 7, 2013 at 4:56 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
>> On 02/07/2013 08:44 PM, Grant Likely wrote:
>>> So, if I'm correct that means that for the data port (specifically
>>> copying between RAM and the data port) using ioread16/iowrite16 on the
>>> data port results in the correct behaviour. It also means that LE
>>> support in the current driver is broken.
>>
>> That matches my earlier note when I wrote that for correct work on LE (note
>> I'm on ARC, not PPC/MB) I needed to use "io{read|write}16" in
>> "ace_data{in|out}_le16".
>>
>> With original "io{read|write}16be" instead data was corrupted.
>
> In which case your bug-fix patch should drop the
> ace_datain_le16/ace_dataout_le16 variants entirely and use the be16
> ones for both (renaming appropriately).
>
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>

Sorry, do you mean to replace original lines:
=======
static void ace_datain_be16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *dst = ace->data_ptr;
	while (i--)
		*dst++ = in_le16(ace->baseaddr + 0x40);
	ace->data_ptr = dst;
}

static void ace_dataout_be16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *src = ace->data_ptr;
	while (i--)
		ioread16(*src++, ace->baseaddr + 0x40);
	ace->data_ptr = src;
}
=======

with something like:
=======
static void ace_datain_16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *dst = ace->data_ptr;
	while (i--)
		*dst++ = in_le16(ace->baseaddr + 0x40);
	ace->data_ptr = dst;
}

static void ace_dataout_16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *src = ace->data_ptr;
	while (i--)
		iowrite16(*src++, ace->baseaddr + 0x40);
	ace->data_ptr = src;
}
=======

and then the next patch should replace "io{read|write}16" with 
"io{read|write}16_rep" I guess, correct?

-Alexey

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Thu, Feb 7, 2013 at 5:22 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> On 02/07/2013 09:16 PM, Grant Likely wrote:
>>
>> On Thu, Feb 7, 2013 at 4:56 PM, Alexey Brodkin
>> <Alexey.Brodkin@synopsys.com> wrote:
>>>
>>> On 02/07/2013 08:44 PM, Grant Likely wrote:
>>>>
>>>> So, if I'm correct that means that for the data port (specifically
>>>> copying between RAM and the data port) using ioread16/iowrite16 on the
>>>> data port results in the correct behaviour. It also means that LE
>>>> support in the current driver is broken.
>>>
>>>
>>> That matches my earlier note when I wrote that for correct work on LE
>>> (note
>>> I'm on ARC, not PPC/MB) I needed to use "io{read|write}16" in
>>> "ace_data{in|out}_le16".
>>>
>>> With original "io{read|write}16be" instead data was corrupted.
>>
>>
>> In which case your bug-fix patch should drop the
>> ace_datain_le16/ace_dataout_le16 variants entirely and use the be16
>> ones for both (renaming appropriately).
>>
>> g.
>>
>> --
>> Grant Likely, B.Sc., P.Eng.
>> Secret Lab Technologies Ltd.
>>
>
> Sorry, do you mean to replace original lines:
> =======
>
> static void ace_datain_be16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *dst = ace->data_ptr;
>         while (i--)
>                 *dst++ = in_le16(ace->baseaddr + 0x40);
>         ace->data_ptr = dst;
>
> }
>
> static void ace_dataout_be16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *src = ace->data_ptr;
>         while (i--)
>                 ioread16(*src++, ace->baseaddr + 0x40);
>         ace->data_ptr = src;
> }
> =======
>
> with something like:
> =======
> static void ace_datain_16(struct ace_device *ace)
>
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *dst = ace->data_ptr;
>         while (i--)
>                 *dst++ = in_le16(ace->baseaddr + 0x40);
>         ace->data_ptr = dst;
> }
>
> static void ace_dataout_16(struct ace_device *ace)
>
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *src = ace->data_ptr;
>         while (i--)
>                 iowrite16(*src++, ace->baseaddr + 0x40);
>         ace->data_ptr = src;
> }

Ummm, I think you finger fumbled that because the above doesn't make sense.

I think that your original patch should be applied as-is first. It is
just a mechanical replacement of the ppc accessors with ioread/iowrite
variants. Nothing controversial there.

I was suggesting to use a second patch to drop
ace_datain_le16/ace_dataout_le16 and rename
ace_datain_be16/ace_dataout_be16 to ace_datain_16/ace_dataout_16.\,
and in that same patch you can switch the read loop to use
ioread16_rep/iowrite16_rep.

*However*... I think my first analysis is actually wrong for the BE
case. The current BE code works using ioread16() for the data port
which does a swap, but ioread16_rep() doesn't swap . I hadn't gone an
actually looked at how the _rep variants are implemented. (Thanks for
your help Ben). That means ioread16_rep will work fine for LE, but
we're still stuck with the slow loop on BE.

So, craft your bug fix for the LE case to use _rep variants for
ace_{datain,dataout}_le16() but don't change the BE support yet.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Thu, 2013-02-07 at 20:56 +0400, Alexey Brodkin wrote:
> > So, if I'm correct that means that for the data port (specifically
> > copying between RAM and the data port) using ioread16/iowrite16 on the
> > data port results in the correct behaviour. It also means that LE
> > support in the current driver is broken.
> 
> That matches my earlier note when I wrote that for correct work on LE 
> (note I'm on ARC, not PPC/MB) I needed to use "io{read|write}16" in 
> "ace_data{in|out}_le16".
> 
> With original "io{read|write}16be" instead data was corrupted.

For the "backward wiring case" your data port will always be the opposite
endian as your host. For the "correct wiring" case, the data port will always
be the same endian as your host.

So the correct wiring case (the one using ioread16/iowrite16 for registers),
the data port should just use ioread16_rep.

For the other case, you need some ifdef'ery or raw variants with home made
barriers, or a bounce buffer.

Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Thu, 2013-02-07 at 16:44 +0000, Grant Likely wrote:
> 
> I've just spent some quality time with a piece of paper, and I think
> I've figured it out...

http://linuxplumbers.ubicast.tv/videos/big-and-little-endian-inside-out/

Watch the last part on IO busses....

This all has to do which which byte of the bus is the lowest byte
*address* regardless of significance. That's how the wiring should be
consistent between CPU and device, in order for a data port to work
properly.

The data port then requires no swapping (which also means it works
nicely with dumb DMA engines etc...)

Whether the registers need swapping or not depends on which half is the
MSB, which is somewhat a semantically higher level than the bus
transport, and depends on whether the device exposes them as BE or LE
registers. But data ports are just "windows" to a byte stream and
shouldn't be affected by endianess (again, unless you get a moron doing
the HW which seems to be still too common).

There is only one right way to connect devices to CPUs basically, which
is called byte address invariance, and preserves the order of bytes in
term of byte addresses.

(Note that for busses that also carry addresses such as PCI, this can
get tricky as you might need to have the lanes routed in a different
order for address vs. data cycles).

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Thursday 07 February 2013, Grant Likely wrote:
> On Thu, Feb 7, 2013 at 3:28 PM, Alexey Brodkin

> Starting with register (non-data) access. The bus bindings are such
> that on both BE and LE systems a native 16 bit read results in the
> bits being in the correct order. On powerpc, you want to do a BE read
> because the device appears as a BE device, and on LE systems a LE read
> is wanted because that is how the device is wired. So far this makes
> sense and matches the driver.

Well, until you get a little-endian PowerPC system with this
device ;). I heard people are plannng to put those into
production systems. I would assume that the device would
still use big-endian registers, so the rule about PowerPC
using the BE accessors still holds, but it stops making
sense.

> The same is true for the data port. A BE read does the right thing on
> a BE platform, and LE read on a LE platform. (again, this is all about
> bus attachment). However, the difference is what is then done with the
> data.

Well, except that we cannot use the ioread16be_rep function,
which is made for the case where the bus does not swap.

Of course, as long as the driver is only ever used to access
the same non-removable block device and you don't change
the driver, it does not matter at all whether you swap bytes
on the data port or not, because they are swapped on both
read and write, and it's just storage. Only if you try to
read the device with a "correct" driver, you will see a problem
if it was written with a "wrong" driver.

> The ironic thing is that if the BE PPC/MB hardware engineers hadn't
> swapped the bytelanes then we would *still* have to do special thinks
> in the hardware; except it would be the data port accesses that would
> need extra work, instead of the other registers.

No, we would juse use the _rep() functions if they had done it right,
just like any other driver with this problem.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Geert Uytterhoeven]

On Fri, Feb 8, 2013 at 12:05 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> The same is true for the data port. A BE read does the right thing on
>> a BE platform, and LE read on a LE platform. (again, this is all about
>> bus attachment). However, the difference is what is then done with the
>> data.
>
> Well, except that we cannot use the ioread16be_rep function,

ioread16_rep

> which is made for the case where the bus does not swap.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Grant Likely]

On Thu, Feb 7, 2013 at 11:05 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 07 February 2013, Grant Likely wrote:
>> On Thu, Feb 7, 2013 at 3:28 PM, Alexey Brodkin
> Of course, as long as the driver is only ever used to access
> the same non-removable block device and you don't change
> the driver, it does not matter at all whether you swap bytes
> on the data port or not, because they are swapped on both
> read and write, and it's just storage. Only if you try to
> read the device with a "correct" driver, you will see a problem
> if it was written with a "wrong" driver.

It's actually a removable compact flash card interface. I know that
the current big-endian support code is correct because my laptop
doesn't complain about the data.  :-)

g.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Thu, 2013-02-07 at 15:23 +0000, Grant Likely wrote:
> 
> Maybe. In a separate patch. Hmmm... I guess there isn't an
> ioread16be_rep variant. 

Because it would not make sense. The ioread16_rep isn't "LE" ... it
should be usable for any kind if data port since such a fifo should
never require byteswap as long as the bus is wired properly (whether the
device is LE or BE).

The problem we have here is that we have an LE device that can be wired
backward. The fact that this only happens when the CPU is BE is somewhat
irrelevant (well, let's say that the confusion is easier to make for the
HW guys when using a BE CPU but fundamentally it's not relevant, a
similar backard wiring could have been done for a BE device on a LE CPU
for example). 

So in that case, you need some kind of hand made loop.

> Oh well. Check first with Michal on LE microblaze before making the
> change. If it doesn't work for him the more understanding is needed. I
> was pretty sure the LE variant already worked.
> 
> > not sure about items for "ace_datain/out_be16" - what about _rep
> options
> > here?
> 
> ioread16_rep should be fine. The ace_data{in,out}_be16 routines need
> to use the LE accessor. The existing code is definitely correct in
> this respect.

Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/7 Grant Likely <grant.likely@secretlab.ca>:
> On Thu, Feb 7, 2013 at 3:12 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
>> On 02/07/2013 06:51 PM, Grant Likely wrote:
>>>
>>> On Thu, Feb 7, 2013 at 2:39 PM, Grant Likely <grant.likely@secretlab.ca>
>>> wrote:
>>>>
>>>> On Wed, Feb 6, 2013 at 9:35 PM, Benjamin Herrenschmidt
>>>> <benh@kernel.crashing.org> wrote:
>>>>>>
>>>>>>   In fact, the driver already knows about this and figures
>>>>>> out at runtime how the device is wired up to the bus. This is not the
>>>>>> problem.
>>>>>
>>>>>
>>>>> Except that this is very gross, especially when you observe that in the
>>>>> busted "big endian" case, it has to byteswap the bloody data port.
>>>>>
>>>>> So you end up having to do that gross hack with separate accessors for
>>>>> registers vs. data and not able to use the _rep variants, which also
>>>>> means that on platforms like ppc, you end up with a memory barrier on
>>>>> every access (or more), which is going to slow things down enormously.
>>>>
>>>>
>>>> I don't see why the _rep variants aren't usable here. The only reason
>>>> I didn't use them when I wrote the driver in the first place was I was
>>>> a n00b kernel hacker and I didn't know they were there.
>>>
>>>
>>> The 8-bit variant is different though because the hardware requires
>>> pingponging between odd and even byte addresses to flush the fifo.
>>> Reading a data port even address (0x40) gives the least significant
>>> byte. Reading from an odd address (0x41) give the MSB and pops the
>>> data off the FIFO. So, yes, the _rep variant can't be used in 8-bit
>>> mode. It should still be fine in 16-bit.
>>>
>>> page 45: http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf
>>>
>>
>> Ok, so may I do a re-spin with these changes:
>
> There are two things here. 1) changing the accessors used. 2)
> switching the endianess as a bug fix. Any changes to the endian access
> should be a separate patch which a description of what is needed.
>
>> 1. In "ace_in_be16" use "ioread16be"
>> 2. In "ace_out_be16" use "iowrite16be"
>> 3. In "ace_in_le16" use "ioread16"
>> 4. In "ace_out_le16" use "iowrite16"
>
> Yes
>
>> 5. In "ace_datain_le16" use "ioread16_rep"
>> 6. In "ace_dataout_le16" use "iowrite16_rep"
>
> Maybe. In a separate patch. Hmmm... I guess there isn't an
> ioread16be_rep variant. Oh well. Check first with Michal on LE
> microblaze before making the change. If it doesn't work for him the
> more understanding is needed. I was pretty sure the LE variant already
> worked.

Sorry it took me some time to get 16bit LE to work for test but here
are test results.

I have tested it on ppc BE, Microblaze BE and Microblaze LE systems
and surprisingly on all of them only ace_reg_le16_ops are used.

But on Microblaze LE is necessary to use different datain/out_le16
functions as below
which are also not compatible with Microblaze and PPC BE.

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index bbad046..8dd192c 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -314,7 +314,7 @@ static void ace_datain_le16(struct ace_device *ace)
        int i = ACE_FIFO_SIZE / 2;
        u16 *dst = ace->data_ptr;
        while (i--)
-               *dst++ = ioread16be(ace->baseaddr + 0x40);
+               *dst++ = ioread16(ace->baseaddr + 0x40);
        ace->data_ptr = dst;
 }

@@ -323,7 +323,7 @@ static void ace_dataout_le16(struct ace_device *ace)
        int i = ACE_FIFO_SIZE / 2;
        u16 *src = ace->data_ptr;
        while (i--)
-               iowrite16be(*src++, ace->baseaddr + 0x40);
+               iowrite16(*src++, ace->baseaddr + 0x40);
        ace->data_ptr = src;
 }

Grant: How did you tested BE mode? It is fpga it means you can switch
some connections
and it could work.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Tuesday 12 February 2013, Michal Simek wrote:
> But on Microblaze LE is necessary to use different datain/out_le16
> functions as below
> which are also not compatible with Microblaze and PPC BE.
> 
> diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
> index bbad046..8dd192c 100644
> --- a/drivers/block/xsysace.c
> +++ b/drivers/block/xsysace.c
> @@ -314,7 +314,7 @@ static void ace_datain_le16(struct ace_device *ace)
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *dst = ace->data_ptr;
>         while (i--)
> -               *dst++ = ioread16be(ace->baseaddr + 0x40);
> +               *dst++ = ioread16(ace->baseaddr + 0x40);
>         ace->data_ptr = dst;
>  }

Hmm, that actually seems sane then: You can replace the loop around
ioread16be with a single ioread32_rep() call, which will use
the correct native endian accesses on both LE Microblaze
and on all the big-endian platforms. Note that the asm-generic
definition of that function was broken until quite recently
when Will Deacon repaired it in order to support ARM64.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Alexey Brodkin]

On 02/12/2013 09:25 PM, Arnd Bergmann wrote:
> On Tuesday 12 February 2013, Michal Simek wrote:
>> But on Microblaze LE is necessary to use different datain/out_le16
>> functions as below
>> which are also not compatible with Microblaze and PPC BE.
>>
>> diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
>> index bbad046..8dd192c 100644
>> --- a/drivers/block/xsysace.c
>> +++ b/drivers/block/xsysace.c
>> @@ -314,7 +314,7 @@ static void ace_datain_le16(struct ace_device *ace)
>>          int i = ACE_FIFO_SIZE / 2;
>>          u16 *dst = ace->data_ptr;
>>          while (i--)
>> -               *dst++ = ioread16be(ace->baseaddr + 0x40);
>> +               *dst++ = ioread16(ace->baseaddr + 0x40);
>>          ace->data_ptr = dst;
>>   }
>
> Hmm, that actually seems sane then: You can replace the loop around
> ioread16be with a single ioread32_rep() call, which will use
> the correct native endian accesses on both LE Microblaze
> and on all the big-endian platforms. Note that the asm-generic
> definition of that function was broken until quite recently
> when Will Deacon repaired it in order to support ARM64.
>
> 	Arnd
>

It seems like this thread was not updated for many weeks now.
I'm wondering if I missed something or if I may do something to help you 
guys move forward with proposed patch?

-Alexey

xsysace driver support on arches other than PPC/Microblaze

[Alexey Brodkin]

Hi all,

I've been trying to get "xsysace" driver working properly on ARC 
architecture.
And I was able to get it built and running, but it required me to do 2 
changes - please refer to description below. Now I'd like to get this 
driver working for me righ from upstream sources and this is where my 
questions appear.

While in general I believe any device driver should be easily build and 
then used on virtually any architecture I faced 2 issues with "xsysace":

1. It uses PPC/Microblaze specific accessors "{in|out}_{le|be}16".
I sent a patch to this mailing list which replaces these custom 
accessors with common "ioread16{|be}" but a long discussion silently 
ended up with nothing (at least for proposed patch).
Patch and discussion is available here: 
https://patchwork.kernel.org/patch/2062701/

2. Inverted logic in "ace_data{in|out}_{be|le}16".
For example in "ace_dataout_le16" "out_be16" accessor is used which 
simply doesn't work for ARC. We need to have "le" accessor in "le" 
function and vice versa. Implementation of "be" ARC accessor with 
inverted logic doesn't help. Because "ace_{in|out}_{be|le}16" uses plain 
logic.

So I'm wondering what is a best way for me to proceed?

For (1) we may implement custom "{in|out}_{le|be}16" accessors, but is 
it a good way to go?
For (2) I may expect that plain change of used accessor from "le" to 
"be" will break "xsysace" on PPC/Micropblaze.

I would like to get some suggestions/proposals from all interested parties.

Regards,
Alexey

Re: xsysace driver support on arches other than PPC/Microblaze

[Alexey Brodkin]

On 06/19/2013 12:56 PM, Alexey Brodkin wrote:
[]
> For (2) I may expect that plain change of used accessor from "le" to
> "be" will break "xsysace" on PPC/Micropblaze.

Just recalled that discussion of the first patch continued in another 
thread: https://patchwork.kernel.org/patch/2130081/

It has a snipped of proposed change for (2).

but this thread as well ended up without any action.

Regards,
Alexey

Re: xsysace driver support on arches other than PPC/Microblaze

[Andy Shevchenko]

On Wed, Jun 19, 2013 at 11:56 AM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> Hi all,
>
> I've been trying to get "xsysace" driver working properly on ARC
> architecture.
> And I was able to get it built and running, but it required me to do 2
> changes - please refer to description below. Now I'd like to get this
> driver working for me righ from upstream sources and this is where my
> questions appear.
>
> While in general I believe any device driver should be easily build and
> then used on virtually any architecture I faced 2 issues with "xsysace":
>
> 1. It uses PPC/Microblaze specific accessors "{in|out}_{le|be}16".
> I sent a patch to this mailing list which replaces these custom
> accessors with common "ioread16{|be}" but a long discussion silently
> ended up with nothing (at least for proposed patch).
> Patch and discussion is available here:
> https://patchwork.kernel.org/patch/2062701/
>
> 2. Inverted logic in "ace_data{in|out}_{be|le}16".
> For example in "ace_dataout_le16" "out_be16" accessor is used which
> simply doesn't work for ARC. We need to have "le" accessor in "le"
> function and vice versa. Implementation of "be" ARC accessor with
> inverted logic doesn't help. Because "ace_{in|out}_{be|le}16" uses plain
> logic.
>
> So I'm wondering what is a best way for me to proceed?
>
> For (1) we may implement custom "{in|out}_{le|be}16" accessors, but is
> it a good way to go?
> For (2) I may expect that plain change of used accessor from "le" to
> "be" will break "xsysace" on PPC/Micropblaze.

If you have a way to detect endianess run-time then you have to
introduce kinda ops structure

struct access {
 .read = read_func(),
 .write = write_func(),
};

ops_le = {...};
ops_be = {...};

And provide a pointer to the chosen structure.

If there no posibility to get it run-time, use kernel config option.

I wonder if there is any "modern" way to do such things.

--
With Best Regards,
Andy Shevchenko

Re: xsysace driver support on arches other than PPC/Microblaze

[Alexey Brodkin]

On 06/19/2013 04:13 PM, Andy Shevchenko wrote:
> On Wed, Jun 19, 2013 at 11:56 AM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
[]
> If you have a way to detect endianess run-time then you have to
> introduce kinda ops structure
>
> struct access {
>   .read = read_func(),
>   .write = write_func(),
> };
>
> ops_le = {...};
> ops_be = {...};
>
> And provide a pointer to the chosen structure.
>
> If there no posibility to get it run-time, use kernel config option.
>
> I wonder if there is any "modern" way to do such things.

Andy, if I understand your idea correctly then this kind of 
functionality is already there.

============
static struct ace_reg_ops ace_reg_be16_ops = {
	.in = ace_in_be16,
	.out = ace_out_be16,
	.datain = ace_datain_be16,
	.dataout = ace_dataout_be16,
};

static struct ace_reg_ops ace_reg_le16_ops = {
	.in = ace_in_le16,
	.out = ace_out_le16,
	.datain = ace_datain_le16,
	.dataout = ace_dataout_le16,
};
============
And in run-time we select which one to use. So no problem here.

The problem I'm facing there is a bit more complex.

Please refer to an extract below:
============
static void ace_out_be16(struct ace_device *ace, int reg, u16 val)
{
	out_be16(ace->baseaddr + reg, val);
}

static void ace_dataout_be16(struct ace_device *ace)
{
	int i = ACE_FIFO_SIZE / 2;
	u16 *src = ace->data_ptr;
	while (i--)
		out_le16(ace->baseaddr + 0x40, *src++);
	ace->data_ptr = src;
}
============

 From it you may see that one high-level big-endian accessor 
("ace_out_be16") uses big-endian low-level accessor ("out_be16") while 
another high-level big-endian accessor ("ace_dataout_be16") uses 
little-endian low-level accessor ("out_be16").

It seems like access to 16-bit data words should be done always with LE 
accessors (after all it's always just a window to a device's memory).

-Alexey

Re: xsysace driver support on arches other than PPC/Microblaze

[Arnd Bergmann]

On Wednesday 19 June 2013, Alexey Brodkin wrote:
> ============
> static void ace_out_be16(struct ace_device *ace, int reg, u16 val)
> {
>         out_be16(ace->baseaddr + reg, val);
> }
> 
> static void ace_dataout_be16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *src = ace->data_ptr;
>         while (i--)
>                 out_le16(ace->baseaddr + 0x40, *src++);
>         ace->data_ptr = src;
> }
> ============
> 
>  From it you may see that one high-level big-endian accessor 
> ("ace_out_be16") uses big-endian low-level accessor ("out_be16") while 
> another high-level big-endian accessor ("ace_dataout_be16") uses 
> little-endian low-level accessor ("out_be16").
> 
> It seems like access to 16-bit data words should be done always with LE 
> accessors (after all it's always just a window to a device's memory).

This is a very long story, but I think the code is right. The point is
that ace_out_be16 accesses a single register that is defined as big-endian,
while ace_dataout_be16 accesses a byte stream in 16-bit units but has to
copy each byte in the same order that they are stored in memory on the
CPU. It usually takes me half an hour to wrap my head around this,
but yes it is this crazy and a lot of drivers do the same thing.

To simplify the above, ace_dataout_be16 should just call ioread16_rep()
which does the loop in the right endianess, while ace_out_be16
could be changed to use iowrite16_be(), which is the similar to
out_be16 but more portable.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Benjamin Herrenschmidt]

On Thu, 2013-02-07 at 19:12 +0400, Alexey Brodkin wrote:
> not sure about items for "ace_datain/out_be16" - what about _rep
> options here?

Well, you have a backward wiring of an LE device so you can't use the
_rep variants, unless you ping pong, so you either use a loop of
ioread/write16 (le) and bite the bullet on extra barriers, or use _rep &
bounce buffer for a separate swap pass.

Point is, the backward wiring will require byteswap on both BE and LE
hosts for data (which is why it's so stupid).

Cheers,
Ben.

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Tuesday 05 February 2013, Michal Simek wrote:
> I want to be sure about this. I have parsed this again with closer look and
> seems to me that ioread32 is equal to readl and iowrite32 to writel.
> Arnd: Am I right?

Correct. On all the architectures you care about (most importantly, not
x86), readl and ioread32 are defined to have the same semantics. There
are a few exceptions where ioread32 provides a wrapper for PCI
PIO accesses that are not memory mapped, making ioread32 slightly
slower than readl, but for all practical purposes you don't have
to worry about it ;-)

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Geert Uytterhoeven]

On Wed, Feb 6, 2013 at 12:03 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 05 February 2013, Michal Simek wrote:
>> I want to be sure about this. I have parsed this again with closer look and
>> seems to me that ioread32 is equal to readl and iowrite32 to writel.
>> Arnd: Am I right?
>
> Correct. On all the architectures you care about (most importantly, not
> x86), readl and ioread32 are defined to have the same semantics. There
> are a few exceptions where ioread32 provides a wrapper for PCI
> PIO accesses that are not memory mapped, making ioread32 slightly
> slower than readl, but for all practical purposes you don't have
> to worry about it ;-)

PCI PIO? Do you mean "PCI I/O space"?
That's different, and accessed through inb() and friends, right?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Arnd Bergmann]

On Wednesday 06 February 2013, Geert Uytterhoeven wrote:
> On Wed, Feb 6, 2013 at 12:03 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 05 February 2013, Michal Simek wrote:
> >> I want to be sure about this. I have parsed this again with closer look and
> >> seems to me that ioread32 is equal to readl and iowrite32 to writel.
> >> Arnd: Am I right?
> >
> > Correct. On all the architectures you care about (most importantly, not
> > x86), readl and ioread32 are defined to have the same semantics. There
> > are a few exceptions where ioread32 provides a wrapper for PCI
> > PIO accesses that are not memory mapped, making ioread32 slightly
> > slower than readl, but for all practical purposes you don't have
> > to worry about it ;-)
> 
> PCI PIO? Do you mean "PCI I/O space"?

Yes.

> That's different, and accessed through inb() and friends, right?

Sort of. The native methods are readl() etc for MMIO and inb() etc. for PIO,
as you say. However, the ioread32() family does the superset of the two,
either using a wrapper function with a mangled port number / pointer
argument (e.g. on x86), or it gets defined to do the same as readl on
platforms where the PCI I/O space is memory mapped. The requirement
here is that whatever the result of ioport_map() is must be turned
back into a PIO access when passed into ioread32(). On memory mapped
platforms, there is typically just an offset that gets added to the
port number to turn that into an __iomem pointer.

	Arnd

Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be)

[Michal Simek]

2013/2/4 Arnd Bergmann <arnd@arndb.de>:
> On Monday 04 February 2013, Michal Simek wrote:
>> >
>> > and select the CONFIG_FOO_BIG_ENDIAN and CONFIG_FOO_LITTLE_ENDIAN
>> > symbols in Kconfig based on the system you are building for.
>>
>> Using CONFIG_FOO_BIG/LITTLE is not good because it is just another
>> Kconfig option.
>> You can easily detect it at runtime and for dedicated hw with fixed
>> endians you can just
>> handle it in the driver and don't care about global setting.
>
> The configuration option should not be visible, so it's
> not something a user would have to worry about. It's just
> sometimes nicer to express the configuration of the platform
> in terms of Kconfig syntax than it is in C code if you have
> complex platform dependencies.
>
>> > This of course gets further complicated if you require different
>> > accessors per architecture, like ARM wanting readl or ioread32
>> > and PowerPC wanting in_le32 for a little-endian SoC component.
>>
>> FYI: I have got two responses on linux-arch from Alan
>> "Set the pointers up and pass them as data with your platform device, that
>> way the function definitions are buried in your platform code where they
>> depend."
>>
>> and Geert:
>> "Or embed a struct io_ops * in struct device, to be set up by the bus driver?
>>
>> Wasn't David Hinds working on something like this in the context of PCMCIA
>> a few decades ago?"
>>
>> Based on their suggestions one way can be to pass it through void *platform_data
>> which is probably not the best and then which make more sense to me is to extend
>> struct dev_archdata archdata to add there native read/write functions.
>>
>> What do you think?
>
> I worry a little about code size if we have a lot of drivers that go
> from one instruction to an indirect function call for each readl/writel.
> Using platform_data ss also something that does not work too well with
> device tree based platform configuration, which tries hard to leave
> all run-time configuration inside of the driver.

As you have seen from the list of architectures all of them uses device tree.

Thanks,
Michal



-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform