diff mbox

[U-Boot] drivers/block/systemace: fixes read/write words for bus width = 8

Message ID 1357137512-8618-1-git-send-email-alexey.brodkin@gmail.com
State Superseded
Delegated to: Wolfgang Denk
Headers show

Commit Message

Alexey Brodkin Jan. 2, 2013, 2:38 p.m. UTC 
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

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

Comments

Alexey Brodkin Jan. 2, 2013, 3:39 p.m. UTC | #1 
The reason for latter modification is to make code more readable.
Do you think if I may submit this as a cosmetic change separately?


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

> 2013/1/2 Alexey Brodkin <alexey.brodkin@gmail.com>:
> > 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
> >
> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > ---
> >  drivers/block/systemace.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c
> > index 247cf06..88561a7 100644
> > --- a/drivers/block/systemace.c
> > +++ b/drivers/block/systemace.c
> > @@ -66,7 +66,8 @@ static void ace_writew(u16 val, unsigned off)
> >                 writeb(val >> 8, base + off + 1);
> >  #endif
> >         }
> > -       out16(base + off, val);
> > +       else
> > +               out16(base + off, val);
> >  }
>
> yes
>
>
> >  static u16 ace_readw(unsigned off)
> > @@ -78,8 +79,8 @@ static u16 ace_readw(unsigned off)
> >                 return readb(base + off) | (readb(base + off + 1) << 8);
> >  #endif
> >         }
> > -
> > -       return in16(base + off);
> > +       else
> > +               return in16(base + off);
> >  }
>
> No need to use it - because for width=8 it will return above.
>
> M
>
>
>
>
> --
> 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
>
Alexey Brodkin Jan. 2, 2013, 7:30 p.m. UTC | #2 
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
>
Wolfgang Denk Jan. 2, 2013, 8:26 p.m. UTC | #3 
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?

Best regards,

Wolfgang Denk
Michal Simek Jan. 3, 2013, 7:36 a.m. UTC | #4 
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
Wolfgang Denk Jan. 3, 2013, 7:44 a.m. UTC | #5 
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?

Thanks.

Best regards,

Wolfgang Denk
Michal Simek Jan. 3, 2013, 9:39 a.m. UTC | #6 
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
Wolfgang Denk Jan. 3, 2013, 10:31 a.m. UTC | #7 
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.

Best regards,

Wolfgang Denk
Michal Simek Jan. 3, 2013, 10:33 a.m. UTC | #8 
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
Wolfgang Denk Jan. 3, 2013, noon UTC | #9 
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
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> ---
>  drivers/block/systemace.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c
> index 247cf06..88561a7 100644
> --- a/drivers/block/systemace.c
> +++ b/drivers/block/systemace.c
> @@ -66,7 +66,8 @@ static void ace_writew(u16 val, unsigned off)
>  		writeb(val >> 8, base + off + 1);
>  #endif
>  	}
> -	out16(base + off, val);
> +	else
> +		out16(base + off, val);
>  }

I agree with this first chunk.

>  static u16 ace_readw(unsigned off)
> @@ -78,8 +79,8 @@ static u16 ace_readw(unsigned off)
>  		return readb(base + off) | (readb(base + off + 1) << 8);
>  #endif
>  	}
> -
> -	return in16(base + off);
> +	else
> +		return in16(base + off);
>  }

But please drop this second one; the "else" is not necessary and adds
only unneeded nesting.

Best regards,

Wolfgang Denk
Alexey Brodkin Jan. 3, 2013, 12:19 p.m. UTC | #10 
Hi Wolfgang,

I've already sent V2 patch with only 1 chunk and updated comments to the
mailing list.

-Alexey
03.01.2013 16:00 пользователь "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
> >
> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > ---
> >  drivers/block/systemace.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c
> > index 247cf06..88561a7 100644
> > --- a/drivers/block/systemace.c
> > +++ b/drivers/block/systemace.c
> > @@ -66,7 +66,8 @@ static void ace_writew(u16 val, unsigned off)
> >               writeb(val >> 8, base + off + 1);
> >  #endif
> >       }
> > -     out16(base + off, val);
> > +     else
> > +             out16(base + off, val);
> >  }
>
> I agree with this first chunk.
>
> >  static u16 ace_readw(unsigned off)
> > @@ -78,8 +79,8 @@ static u16 ace_readw(unsigned off)
> >               return readb(base + off) | (readb(base + off + 1) << 8);
> >  #endif
> >       }
> > -
> > -     return in16(base + off);
> > +     else
> > +             return in16(base + off);
> >  }
>
> But please drop this second one; the "else" is not necessary and adds
> only unneeded nesting.
>
> 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
> If programming was easy, they wouldn't need something as  complicated
> as a human being to do it, now would they?
>                        - L. Wall & R. L. Schwartz, _Programming Perl_
>
diff mbox

Patch

diff --git a/drivers/block/systemace.c b/drivers/block/systemace.c
index 247cf06..88561a7 100644
--- a/drivers/block/systemace.c
+++ b/drivers/block/systemace.c
@@ -66,7 +66,8 @@  static void ace_writew(u16 val, unsigned off)
 		writeb(val >> 8, base + off + 1);
 #endif
 	}
-	out16(base + off, val);
+	else
+		out16(base + off, val);
 }
 
 static u16 ace_readw(unsigned off)
@@ -78,8 +79,8 @@  static u16 ace_readw(unsigned off)
 		return readb(base + off) | (readb(base + off + 1) << 8);
 #endif
 	}
-
-	return in16(base + off);
+	else
+		return in16(base + off);
 }
 
 static unsigned long systemace_read(int dev, unsigned long start,