diff mbox

[U-Boot] drivers/block/systemace: replaced in16/out16 with more common readw/writew macros

Message ID 1357139186-9172-1-git-send-email-alexey.brodkin@gmail.com
State Rejected
Delegated to: Wolfgang Denk
Headers show

Commit Message

Alexey Brodkin Jan. 2, 2013, 3:06 p.m. UTC 
Most architectures don't have symbols "in16"/"out16" defined.
Only Microblaze/PowerPC/Spark architectures do have them defined.

At the same time there're much more common macros "readw"/"writew" for
16-bit data access defined in most of architectures (in
linux kernel header "io.h").

So use of "readw"/"writew" makes it possible to build this driver for
virtually any architecture.

Signed-off-by: Alexey Brodkin <alexey.brodkin@gmail.com>
---
 drivers/block/systemace.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Wolfgang Denk Jan. 2, 2013, 8:23 p.m. UTC | #1 
Dear Alexey Brodkin,

In message <1357139186-9172-1-git-send-email-alexey.brodkin@gmail.com> you wrote:
> Most architectures don't have symbols "in16"/"out16" defined.
> Only Microblaze/PowerPC/Spark architectures do have them defined.
> 
> At the same time there're much more common macros "readw"/"writew" for
> 16-bit data access defined in most of architectures (in
> linux kernel header "io.h").
> 
> So use of "readw"/"writew" makes it possible to build this driver for
> virtually any architecture.
> 
> Signed-off-by: Alexey Brodkin <alexey.brodkin@gmail.com>

Did you actually test this on - say - any PowerPC board?

To my understanding, in16() / out16() perform big endian accesses 
(i. e. direct load / store operations) on PowerPC, while readw /
writew do byte-swapping (for accessling little-endian things like PCI
busses).

I speculate that this patch will break all big-endian systems.

Best regards,

Wolfgang Denk
Alexey Brodkin Jan. 3, 2013, 11:35 a.m. UTC | #2 
Unfortunately I didn't test it on any BE hardware - I don't have
anything handy at the moment.
I got it tested on LE platform - though it has nothing to do with real
BE behavior.

But as far as I can see from kernel headers "writew" does this -
regardless CPU endianess it writes 16-bit word to specified memory
location in LE order.
http://lxr.free-electrons.com/source/arch/microblaze/include/asm/io.h#L96
====
static inline void writew(unsigned short v, volatile void __iomem *addr)
{
         *(volatile unsigned short __force *)addr = cpu_to_le16(v);
}
====

So depending on SystemACE internal endianess we may or may not use "writew".
If SystemACE is always LE then "writew"/"readw" will do its work as expected.
If SystemACE has the same endianess as CPU does then we may use
"__raw_writew"/"__raw_readw" for data access.

But here http://www.xilinx.com/support/documentation/ip_documentation/xps_sysace.pdf
it is said:
====
The Xilinx System ACE Compact Flash chip is a true little-endian device.
====

So I expect "writew"/"readw" to work correctly assuming not
intermediate bridge is put between CPU and System ACE controller.

Would be very nice if Michal may check it on his hardware and let us
know about his findings.

2013/1/3 Wolfgang Denk <wd@denx.de>:
> Dear Alexey Brodkin,
>
> In message <1357139186-9172-1-git-send-email-alexey.brodkin@gmail.com> you wrote:
>> Most architectures don't have symbols "in16"/"out16" defined.
>> Only Microblaze/PowerPC/Spark architectures do have them defined.
>>
>> At the same time there're much more common macros "readw"/"writew" for
>> 16-bit data access defined in most of architectures (in
>> linux kernel header "io.h").
>>
>> So use of "readw"/"writew" makes it possible to build this driver for
>> virtually any architecture.
>>
>> Signed-off-by: Alexey Brodkin <alexey.brodkin@gmail.com>
>
> Did you actually test this on - say - any PowerPC board?
>
> To my understanding, in16() / out16() perform big endian accesses
> (i. e. direct load / store operations) on PowerPC, while readw /
> writew do byte-swapping (for accessling little-endian things like PCI
> busses).
>
> I speculate that this patch will break all big-endian systems.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "There are three principal ways to lose money: wine, women, and engi-
> neers. While the first two are more pleasant, the third is by far the
> more certain."                           - Baron Rothschild, ca. 1800
Wolfgang Denk Jan. 3, 2013, 12:05 p.m. UTC | #3 
Dear Alexey Brodkin,

In message <1357139186-9172-1-git-send-email-alexey.brodkin@gmail.com> you wrote:
> Most architectures don't have symbols "in16"/"out16" defined.
> Only Microblaze/PowerPC/Spark architectures do have them defined.
> 
> At the same time there're much more common macros "readw"/"writew" for
> 16-bit data access defined in most of architectures (in
> linux kernel header "io.h").
> 
> So use of "readw"/"writew" makes it possible to build this driver for
> virtually any architecture.

NAK.  As expected, this breaks big endian systems like PowerPC.

Tested on AMCC "katmai" board (PPC440SPe):

before (current master):

U-Boot 2013.01-rc2-00174-ge56cdd7 (Jan 03 2013 - 12:41:58)
...
=> fatls ace 0 mnt-xsa
            ./
            ../
  1250875   kernel.img 
 19071970   ramdisk.img 

2 file(s), 2 dir(s)


After your patch applied:

U-Boot 2013.01-rc2-00174-ge56cdd7-dirty (Jan 03 2013 - 13:01:27)
...
=> fatls ace 0 mnt-xsa
**** ACE CompactFlash not found.
** Bad device size - ace 0 **


So clear NAK.

Best regards,

Wolfgang Denk
Wolfgang Denk Jan. 3, 2013, 12:37 p.m. UTC | #4 
Dear Alexey,

please don't top post / full quote, and please keep the ML on Cc: - thanks.

In message <CAML3pwX=dfC4vfRfg-Rft5Xuem7q4-7VjtmySvitzWzY3z8JBg@mail.gmail.com> you wrote:
>
> And what if you use "__raw_writew"/"__raw_readw" instead?

I'd rather not try that, as the __raw_* functions are missing the
memory barriers that are mandatory for such device access.

Best regards,

Wolfgang Denk
Michal Simek Jan. 3, 2013, 1:03 p.m. UTC | #5 
2013/1/3 Wolfgang Denk <wd@denx.de>:
> Dear Alexey Brodkin,
>
> In message <1357139186-9172-1-git-send-email-alexey.brodkin@gmail.com> you wrote:
>> Most architectures don't have symbols "in16"/"out16" defined.
>> Only Microblaze/PowerPC/Spark architectures do have them defined.
>>
>> At the same time there're much more common macros "readw"/"writew" for
>> 16-bit data access defined in most of architectures (in
>> linux kernel header "io.h").
>>
>> So use of "readw"/"writew" makes it possible to build this driver for
>> virtually any architecture.
>
> NAK.  As expected, this breaks big endian systems like PowerPC.
>
> Tested on AMCC "katmai" board (PPC440SPe):
>
> before (current master):
>
> U-Boot 2013.01-rc2-00174-ge56cdd7 (Jan 03 2013 - 12:41:58)
> ...
> => fatls ace 0 mnt-xsa
>             ./
>             ../
>   1250875   kernel.img
>  19071970   ramdisk.img
>
> 2 file(s), 2 dir(s)
>
>
> After your patch applied:
>
> U-Boot 2013.01-rc2-00174-ge56cdd7-dirty (Jan 03 2013 - 13:01:27)
> ...
> => fatls ace 0 mnt-xsa
> **** ACE CompactFlash not found.
> ** Bad device size - ace 0 **
>

Just a question is this 8bit width or 16bit?
I have checked with our hw guy and we don't have hw board for 8bit width.

Thanks,
Michal
Wolfgang Denk Jan. 3, 2013, 1:38 p.m. UTC | #6 
Dear Michal Simek,

In message <CAHTX3dLp6AnYpbSz=jFjCZCbZMtAHb9_h-4hcv=VxcZHPnp8bQ@mail.gmail.com> you wrote:
>
> > After your patch applied:
> >
> > U-Boot 2013.01-rc2-00174-ge56cdd7-dirty (Jan 03 2013 - 13:01:27)
> > ...
> > => fatls ace 0 mnt-xsa
> > **** ACE CompactFlash not found.
> > ** Bad device size - ace 0 **
> 
> Just a question is this 8bit width or 16bit?
> I have checked with our hw guy and we don't have hw board for 8bit width.

-> grep CONFIG_SYS_SYSTEMACE_WIDTH include/configs/katmai.h 
#define CONFIG_SYS_SYSTEMACE_WIDTH      16      /* Data bus width is 16         */


Best regards,

Wolfgang Denk
Michal Simek Jan. 3, 2013, 1:53 p.m. UTC | #7 
2013/1/3 Wolfgang Denk <wd@denx.de>:
> Dear Michal Simek,
>
> In message <CAHTX3dLp6AnYpbSz=jFjCZCbZMtAHb9_h-4hcv=VxcZHPnp8bQ@mail.gmail.com> you wrote:
>>
>> > After your patch applied:
>> >
>> > U-Boot 2013.01-rc2-00174-ge56cdd7-dirty (Jan 03 2013 - 13:01:27)
>> > ...
>> > => fatls ace 0 mnt-xsa
>> > **** ACE CompactFlash not found.
>> > ** Bad device size - ace 0 **
>>
>> Just a question is this 8bit width or 16bit?
>> I have checked with our hw guy and we don't have hw board for 8bit width.
>
> -> grep CONFIG_SYS_SYSTEMACE_WIDTH include/configs/katmai.h
> #define CONFIG_SYS_SYSTEMACE_WIDTH      16      /* Data bus width is 16         */

Fair enough. I see that JSE is using 8 bit width - do you have this
board for test?

Thanks,
Michal
Wolfgang Denk Jan. 3, 2013, 2:37 p.m. UTC | #8 
Dear Michal Simek,

In message <CAHTX3d+=yQ2NdTDViEoK3rG7cSqpFuOd8RwgnExpJjANWTd1QQ@mail.gmail.com> you wrote:
>
> > -> grep CONFIG_SYS_SYSTEMACE_WIDTH include/configs/katmai.h
> > #define CONFIG_SYS_SYSTEMACE_WIDTH      16      /* Data bus width is 16         */
> 
> Fair enough. I see that JSE is using 8 bit width - do you have this
> board for test?

Sorry, we don't have any such board.  I think the Katmai is the only
one we have with a SystemACE on it.

Best regards,

Wolfgang Denk
Alexey Brodkin Jan. 4, 2013, 9:29 a.m. UTC | #9 
Hi Wolfgang,

but then do you think if there's any way to use some generic
read/write routines?
The problem is we use ml509 board loaded with Synopsys DW ACR700 core
and we need to access SystemACE CF-card controller (to boot Linux
kernel mainly).

Or should I just add conditional branch for ARC architecture?

Regards,
Alexey

2013/1/3 Wolfgang Denk <wd@denx.de>:
> Dear Alexey,
>
> please don't top post / full quote, and please keep the ML on Cc: - thanks.
>
> In message <CAML3pwX=dfC4vfRfg-Rft5Xuem7q4-7VjtmySvitzWzY3z8JBg@mail.gmail.com> you wrote:
>>
>> And what if you use "__raw_writew"/"__raw_readw" instead?
>
> I'd rather not try that, as the __raw_* functions are missing the
> memory barriers that are mandatory for such device access.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Hegel was right when he said that we learn from history that man  can
> never learn anything from history.              - George Bernard Shaw
Wolfgang Denk Jan. 4, 2013, 10:22 a.m. UTC | #10 
Dear Alexey,

In message <CAML3pwXy7_FiVqJvgmm4-aHbspdY6BnMsZonYfzcw_hA9Fe_fw@mail.gmail.com> you wrote:
> 
> but then do you think if there's any way to use some generic
> read/write routines?

For reference, please see section "KERNEL I/O BARRIER EFFECTS" in the
Linux kernel Documentation/memory-barriers.txt file.  According to
that, we should strive to using ioreadX(), iowriteX() (and some kernel
hackers actually support such an oppinion) - however, there is little
support for thjis in existing code.

ARM traditionally tends to be using readX(), writeX(), at least on the
LE implmentamtions (I don't know what the BE variants do).  PowerPC
has tradtitionally been using inX(), outX() resp. their explicit
counterparts in_leX() / in_beX() / out_leX() / out_beX().  These
functions are also available in ARM, and have been used for drivers
for IP blocks shared between SoCs of different architectures.

> The problem is we use ml509 board loaded with Synopsys DW ACR700 core
> and we need to access SystemACE CF-card controller (to boot Linux
> kernel mainly).
> 
> Or should I just add conditional branch for ARC architecture?

Is there a need to change the driver at all?  Can you not just provide
the needed inX(), outX() accessors for your architecture?

Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c
index 88561a7..32c9169 100644
--- a/drivers/block/systemace.c
+++ b/drivers/block/systemace.c
@@ -67,7 +67,7 @@  static void ace_writew(u16 val, unsigned off)
 #endif
 	}
 	else
-		out16(base + off, val);
+		writew(val, base + off);
 }
 
 static u16 ace_readw(unsigned off)
@@ -80,7 +80,7 @@  static u16 ace_readw(unsigned off)
 #endif
 	}
 	else
-		return in16(base + off);
+		return readw(base + off);
 }
 
 static unsigned long systemace_read(int dev, unsigned long start,