Actually I meant to respin both patches. 1. Only add "else" thread for "ace_writew", keeping "ace_readw" unchanged 2. Replace "in16"/"out16" with "readw"/"writew"

2013/1/2 Michal Simek monstr@monstr.eu

Hi,

2013/1/2 Алексей Бродкин alexey.brodkin@gmail.com:

...

The reason for latter modification is to make code more readable. Do you think if I may submit this as a cosmetic change separately?

If you mean create 3rd patch then no.

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

Hi Wolfgang,

2013/1/2 Wolfgang Denk wd@denx.de:

Dear Alexey Brodkin,

In message 1357137512-8618-1-git-send-email-alexey.brodkin@gmail.com you wrote:

...

From: Alexey Brodkin abrodkin@synopsys.com

Current implementation works fine for bus width = 16 bits because we never get into "if" branch.

If one sets width to 8 bits there will be 2 consequent data accesses (read/write):

1. Correct data access for 8-bit bus
2. Unconditional (and in this case incorrect) data access as if data bus

is 16-bit wide

Sorry, I don't get this.

You say, if one sets width to 8 bits, then the code fails when the bus is actually 16 bits wide?

But this is just a bad configuration then - why do we need to change the code? All that is needed is to run with a correct configuration?

Here is that function with my comment about missing "else". In ace_writew function we shouldn't write 16bit value when width is 8. (It probably doesn't break anything but it is not correct - I will check it on the HW)

static void ace_writew(u16 val, unsigned off) { if (width == 8) { #if !defined(__BIG_ENDIAN) writeb(val >> 8, base + off); writeb(val, base + off + 1); #else writeb(val, base + off); writeb(val >> 8, base + off + 1); #endif }

<----------------------- missing else here writew(base + off, val); }

For ace_readw is situation different because it always returns value for width 8 that's why it never reach 16bit read.

Thanks, Michal

2013/1/3 Wolfgang Denk wd@denx.de:

Dear Michal Simek,

In message CAHTX3d+shCmUjaXzS0oS5pT8a89cyA9UKemc+oRk5xk6qRNrdQ@mail.gmail.com you wrote:

...

...

You say, if one sets width to 8 bits, then the code fails when the bus is actually 16 bits wide?

...

...

Here is that function with my comment about missing "else". In ace_writew function we shouldn't write 16bit value when width is 8. (It probably doesn't break anything but it is not correct - I will check it on the HW)

I see. Well, I think the commit message is not really clear. I definitely misunderstood it. Maybe it can be improved?

Yep.

btw: is there any coding style rule about this second function

Current (simplified) case. static u16 ace_readw(unsigned off) { if (width == 8) return readb(base + off) | (readb(base + off + 1) << 8);

return readw(base + off); }

or (with else)

static u16 ace_readw(unsigned off) { if (width == 8) return readb(base + off) | (readb(base + off + 1) << 8); else return readw(base + off); }

Thanks, Michal

2013/1/3 Wolfgang Denk wd@denx.de:

Dear Michal Simek,

In message CAHTX3dLGmpBLxwYPdK2Fju4J7t8i+55L3h27O2HcxxcvJW-bxA@mail.gmail.com you wrote:

...

...

I see. Well, I think the commit message is not really clear. I definitely misunderstood it. Maybe it can be improved?

Yep.

Thanks.

...

btw: is there any coding style rule about this second function

Current (simplified) case. static u16 ace_readw(unsigned off) { if (width == 8) return readb(base + off) | (readb(base + off + 1) << 8);

  return readw(base + off);

}

or (with else)

static u16 ace_readw(unsigned off) { if (width == 8) return readb(base + off) | (readb(base + off + 1) << 8); else return readw(base + off); }

I am not aware of a CodingStyle rule; in this example there is also not much difference due to the simplicity of the code (note however that indentation of the second return is incorrect in the second example).

My personal preference is the first example, as it uses minimal nesting; I feel this is easier to read.

For me too.

Thanks, Michal