Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-25 Thread Stephen Warren 
On 03/24/2014 08:27 PM, Simon Glass wrote:
 Hi Stephen,
 
 On 21 March 2014 11:28, Stephen Warren swar...@wwwdotorg.org wrote:
 From: Stephen Warren swar...@nvidia.com

 This removes a bunch of open-coded register IO, masking, and shifting.
 I would have squashed this into ARM: tegra: pinctrl: remove duplication
 except that keeping it a separate commit allows easier bisection of any
 issues that are introduced by this patch. I also wrote this patch on top
 of the series, and pushing it any lower in the series results in some
 conflicts I didn't feel like fixing.

 Signed-off-by: Stephen Warren swar...@nvidia.com
 
 Acked-by: Simon Glass s...@chromium.org
 
 But see comment below.

 diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c 
 b/arch/arm/cpu/tegra-common/pinmux-common.c

 +static inline void update_field(u32 *reg, u32 mask, u32 shift, u32 val)
 +{
 +   clrsetbits_le32(reg, mask  shift, val  shift);
 
 I wonder if it would be better to write this out explicitly in each site.
 
 +}
 +
  void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
  {
 u32 *reg = MUX_REG(pin);
 int i, mux = -1;
 -   u32 val;

 /* Error check on pin and func */
 assert(pmux_pingrp_isvalid(pin));
 @@ -110,42 +114,29 @@ void pinmux_set_func(enum pmux_pingrp pin, enum 
 pmux_func func)
 }
 assert(mux != -1);

 -   val = readl(reg);
 -   val = ~(3  MUX_SHIFT(pin));
 -   val |= (mux  MUX_SHIFT(pin));
 -   writel(val, reg);
 +   update_field(reg, 3, MUX_SHIFT(pin), mux);
 
 Because here you are obscuring the shift - the parameter order is by
 no means obvious.

Well, for pretty much no function is it obvious from the function name
what the parameter order is; it's just one of those things you memorize
or look up. The exact same issue exists for clrsetbits_le32() itself IMHO.

 Or perhaps update_reg_mask_shift_val()?

Still, I can rename the function if you want; it certainly does make it
obvious. It's rather a long name though, but I guess wrapping the
parameters isn't too bad.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-25 Thread Simon Glass 
Hi Stephen,

On 25 March 2014 08:54, Stephen Warren swar...@wwwdotorg.org wrote:
 On 03/24/2014 08:27 PM, Simon Glass wrote:
 Hi Stephen,

 On 21 March 2014 11:28, Stephen Warren swar...@wwwdotorg.org wrote:
 From: Stephen Warren swar...@nvidia.com

 This removes a bunch of open-coded register IO, masking, and shifting.
 I would have squashed this into ARM: tegra: pinctrl: remove duplication
 except that keeping it a separate commit allows easier bisection of any
 issues that are introduced by this patch. I also wrote this patch on top
 of the series, and pushing it any lower in the series results in some
 conflicts I didn't feel like fixing.

 Signed-off-by: Stephen Warren swar...@nvidia.com

 Acked-by: Simon Glass s...@chromium.org

 But see comment below.

 diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c 
 b/arch/arm/cpu/tegra-common/pinmux-common.c

 +static inline void update_field(u32 *reg, u32 mask, u32 shift, u32 val)
 +{
 +   clrsetbits_le32(reg, mask  shift, val  shift);

 I wonder if it would be better to write this out explicitly in each site.

 +}
 +
  void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
  {
 u32 *reg = MUX_REG(pin);
 int i, mux = -1;
 -   u32 val;

 /* Error check on pin and func */
 assert(pmux_pingrp_isvalid(pin));
 @@ -110,42 +114,29 @@ void pinmux_set_func(enum pmux_pingrp pin, enum 
 pmux_func func)
 }
 assert(mux != -1);

 -   val = readl(reg);
 -   val = ~(3  MUX_SHIFT(pin));
 -   val |= (mux  MUX_SHIFT(pin));
 -   writel(val, reg);
 +   update_field(reg, 3, MUX_SHIFT(pin), mux);

 Because here you are obscuring the shift - the parameter order is by
 no means obvious.

 Well, for pretty much no function is it obvious from the function name
 what the parameter order is; it's just one of those things you memorize
 or look up. The exact same issue exists for clrsetbits_le32() itself IMHO.

Well that at least indicates that the clr mask comes before set.


 Or perhaps update_reg_mask_shift_val()?

 Still, I can rename the function if you want; it certainly does make it
 obvious. It's rather a long name though, but I guess wrapping the
 parameters isn't too bad.

I'll leave it to you, it's just a thought.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-25 Thread Stephen Warren 
On 03/25/2014 10:04 AM, Simon Glass wrote:
 On 25 March 2014 08:54, Stephen Warren swar...@wwwdotorg.org wrote:
 On 03/24/2014 08:27 PM, Simon Glass wrote:
 On 21 March 2014 11:28, Stephen Warren swar...@wwwdotorg.org wrote:
 This removes a bunch of open-coded register IO, masking, and shifting.
 I would have squashed this into ARM: tegra: pinctrl: remove duplication
 except that keeping it a separate commit allows easier bisection of any
 issues that are introduced by this patch. I also wrote this patch on top
 of the series, and pushing it any lower in the series results in some
 conflicts I didn't feel like fixing.

 Signed-off-by: Stephen Warren swar...@nvidia.com

 Acked-by: Simon Glass s...@chromium.org

 But see comment below.

 diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c 
 b/arch/arm/cpu/tegra-common/pinmux-common.c

 +static inline void update_field(u32 *reg, u32 mask, u32 shift, u32 val)
 +{
 +   clrsetbits_le32(reg, mask  shift, val  shift);

 I wonder if it would be better to write this out explicitly in each site.
...
 +   update_field(reg, 3, MUX_SHIFT(pin), mux);

 Because here you are obscuring the shift - the parameter order is by
 no means obvious.

 Well, for pretty much no function is it obvious from the function name
 what the parameter order is; it's just one of those things you memorize
 or look up. The exact same issue exists for clrsetbits_le32() itself IMHO.
 
 Well that at least indicates that the clr mask comes before set.
 

 Or perhaps update_reg_mask_shift_val()?

 Still, I can rename the function if you want; it certainly does make it
 obvious. It's rather a long name though, but I guess wrapping the
 parameters isn't too bad.
 
 I'll leave it to you, it's just a thought.

Well, the wrapping didn't work out to badly, so I posted V3 including
this rename.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-25 Thread Wolfgang Denk 
Dear Stephen Warren,

In message 5331a6b6.8090...@wwwdotorg.org you wrote:

  Or perhaps update_reg_mask_shift_val()?
 
 Still, I can rename the function if you want; it certainly does make it
 obvious. It's rather a long name though, but I guess wrapping the
 parameters isn't too bad.

Please do not invent new bit manipulation functions.  Just use the
standard I/O accessors.  And whenever possible, please remove pre-
existing functions.

I've just recently sent patches to get rid of such inventions that
resulted in undefined code.

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: w...@denx.de
Faith: not *wanting* to know what is true.- Friedrich Nietzsche
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-25 Thread Stephen Warren 
On 03/25/2014 10:51 AM, Wolfgang Denk wrote:
 Dear Stephen Warren,
 
 In message 5331a6b6.8090...@wwwdotorg.org you wrote:

 Or perhaps update_reg_mask_shift_val()?

 Still, I can rename the function if you want; it certainly does make it
 obvious. It's rather a long name though, but I guess wrapping the
 parameters isn't too bad.
 
 Please do not invent new bit manipulation functions.  Just use the
 standard I/O accessors.  And whenever possible, please remove pre-
 existing functions.
 
 I've just recently sent patches to get rid of such inventions that
 resulted in undefined code.

That's not what this code is doing. The existing IO accessors are used;
it's just removing duplication from the parameters passed to the
existing functions (the shift needs to be written out twice).

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-25 Thread Wolfgang Denk 
Dear Stephen Warren,

In message 5331b4e4.5090...@wwwdotorg.org you wrote:

  Please do not invent new bit manipulation functions.  Just use the
  standard I/O accessors.  And whenever possible, please remove pre-
  existing functions.
  
  I've just recently sent patches to get rid of such inventions that
  resulted in undefined code.
 
 That's not what this code is doing. The existing IO accessors are used;
 it's just removing duplication from the parameters passed to the
 existing functions (the shift needs to be written out twice).

Yes, I've seen that.  You can save a little typ[ing this way.  But you
have to type it only once, and then maintain it forever.  And the
helper function obscures what is happening, which is bad.  It makes
the code more difficult to read and understand and maintain,
especially when the shift count is not easily recognizable - at least I
cannot really read what PULL_SHIFT(pin) decodes to.

Please make the code simple to read, use existing standard functions.



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: w...@denx.de
An armed society is a polite society.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-25 Thread Stephen Warren 
On 03/21/2014 12:28 PM, Stephen Warren wrote:
 From: Stephen Warren swar...@nvidia.com
 
 This removes a bunch of open-coded register IO, masking, and shifting.
 I would have squashed this into ARM: tegra: pinctrl: remove duplication
 except that keeping it a separate commit allows easier bisection of any
 issues that are introduced by this patch. I also wrote this patch on top
 of the series, and pushing it any lower in the series results in some
 conflicts I didn't feel like fixing.

Tom, when applying this series, please just drop this patch. Also,
ignore V3 of this patch. Thanks.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-25 Thread Wolfgang Denk 
Dear Tom,

In message 5331bc48.7020...@wwwdotorg.org Stephen Warren wrote:
 On 03/21/2014 12:28 PM, Stephen Warren wrote:
  From: Stephen Warren swar...@nvidia.com
  
  This removes a bunch of open-coded register IO, masking, and shifting.
  I would have squashed this into ARM: tegra: pinctrl: remove duplication
  except that keeping it a separate commit allows easier bisection of any
  issues that are introduced by this patch. I also wrote this patch on top
  of the series, and pushing it any lower in the series results in some
  conflicts I didn't feel like fixing.
 
 Tom, when applying this series, please just drop this patch. Also,
 ignore V3 of this patch. Thanks.

Alternatively, please consider patch V4 which I just posted.

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: w...@denx.de
One essential to success is that you desire be an all-obsessing  one,
your thoughts and aims be co-ordinated, and your energy be concentra-
ted and applied without letup.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-24 Thread Simon Glass 
Hi Stephen,

On 21 March 2014 11:28, Stephen Warren swar...@wwwdotorg.org wrote:
 From: Stephen Warren swar...@nvidia.com

 This removes a bunch of open-coded register IO, masking, and shifting.
 I would have squashed this into ARM: tegra: pinctrl: remove duplication
 except that keeping it a separate commit allows easier bisection of any
 issues that are introduced by this patch. I also wrote this patch on top
 of the series, and pushing it any lower in the series results in some
 conflicts I didn't feel like fixing.

 Signed-off-by: Stephen Warren swar...@nvidia.com

Acked-by: Simon Glass s...@chromium.org

But see comment below.

 ---
 V2: New patch.
 ---
  arch/arm/cpu/tegra-common/pinmux-common.c | 135 
 +-
  1 file changed, 23 insertions(+), 112 deletions(-)

 diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c 
 b/arch/arm/cpu/tegra-common/pinmux-common.c
 index 32a46d53f068..7f7d4a87a4aa 100644
 --- a/arch/arm/cpu/tegra-common/pinmux-common.c
 +++ b/arch/arm/cpu/tegra-common/pinmux-common.c
 @@ -87,11 +87,15 @@
  #define IO_RESET_SHIFT 8
  #define RCV_SEL_SHIFT  9

 +static inline void update_field(u32 *reg, u32 mask, u32 shift, u32 val)
 +{
 +   clrsetbits_le32(reg, mask  shift, val  shift);

I wonder if it would be better to write this out explicitly in each site.

 +}
 +
  void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
  {
 u32 *reg = MUX_REG(pin);
 int i, mux = -1;
 -   u32 val;

 /* Error check on pin and func */
 assert(pmux_pingrp_isvalid(pin));
 @@ -110,42 +114,29 @@ void pinmux_set_func(enum pmux_pingrp pin, enum 
 pmux_func func)
 }
 assert(mux != -1);

 -   val = readl(reg);
 -   val = ~(3  MUX_SHIFT(pin));
 -   val |= (mux  MUX_SHIFT(pin));
 -   writel(val, reg);
 +   update_field(reg, 3, MUX_SHIFT(pin), mux);

Because here you are obscuring the shift - the parameter order is by
no means obvious.

Or perhaps update_reg_mask_shift_val()?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH V2 09/13] ARM: tegra: use clrsetbits_le32 in pinmux driver

2014-03-21 Thread Stephen Warren 
From: Stephen Warren swar...@nvidia.com

This removes a bunch of open-coded register IO, masking, and shifting.
I would have squashed this into ARM: tegra: pinctrl: remove duplication
except that keeping it a separate commit allows easier bisection of any
issues that are introduced by this patch. I also wrote this patch on top
of the series, and pushing it any lower in the series results in some
conflicts I didn't feel like fixing.

Signed-off-by: Stephen Warren swar...@nvidia.com
---
V2: New patch.
---
 arch/arm/cpu/tegra-common/pinmux-common.c | 135 +-
 1 file changed, 23 insertions(+), 112 deletions(-)

diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c 
b/arch/arm/cpu/tegra-common/pinmux-common.c
index 32a46d53f068..7f7d4a87a4aa 100644
--- a/arch/arm/cpu/tegra-common/pinmux-common.c
+++ b/arch/arm/cpu/tegra-common/pinmux-common.c
@@ -87,11 +87,15 @@
 #define IO_RESET_SHIFT 8
 #define RCV_SEL_SHIFT  9
 
+static inline void update_field(u32 *reg, u32 mask, u32 shift, u32 val)
+{
+   clrsetbits_le32(reg, mask  shift, val  shift);
+}
+
 void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func func)
 {
u32 *reg = MUX_REG(pin);
int i, mux = -1;
-   u32 val;
 
/* Error check on pin and func */
assert(pmux_pingrp_isvalid(pin));
@@ -110,42 +114,29 @@ void pinmux_set_func(enum pmux_pingrp pin, enum pmux_func 
func)
}
assert(mux != -1);
 
-   val = readl(reg);
-   val = ~(3  MUX_SHIFT(pin));
-   val |= (mux  MUX_SHIFT(pin));
-   writel(val, reg);
+   update_field(reg, 3, MUX_SHIFT(pin), mux);
 }
 
 void pinmux_set_pullupdown(enum pmux_pingrp pin, enum pmux_pull pupd)
 {
u32 *reg = PULL_REG(pin);
-   u32 val;
 
/* Error check on pin and pupd */
assert(pmux_pingrp_isvalid(pin));
assert(pmux_pin_pupd_isvalid(pupd));
 
-   val = readl(reg);
-   val = ~(3  PULL_SHIFT(pin));
-   val |= (pupd  PULL_SHIFT(pin));
-   writel(val, reg);
+   update_field(reg, 3, PULL_SHIFT(pin), pupd);
 }
 
 static void pinmux_set_tristate(enum pmux_pingrp pin, int tri)
 {
u32 *reg = TRI_REG(pin);
-   u32 val;
 
/* Error check on pin */
assert(pmux_pingrp_isvalid(pin));
assert(pmux_pin_tristate_isvalid(tri));
 
-   val = readl(reg);
-   if (tri == PMUX_TRI_TRISTATE)
-   val |= (1  TRI_SHIFT(pin));
-   else
-   val = ~(1  TRI_SHIFT(pin));
-   writel(val, reg);
+   update_field(reg, 1, TRI_SHIFT(pin), tri == PMUX_TRI_TRISTATE);
 }
 
 void pinmux_tristate_enable(enum pmux_pingrp pin)
@@ -162,7 +153,6 @@ void pinmux_tristate_disable(enum pmux_pingrp pin)
 void pinmux_set_io(enum pmux_pingrp pin, enum pmux_pin_io io)
 {
u32 *reg = REG(pin);
-   u32 val;
 
if (io == PMUX_PIN_NONE)
return;
@@ -171,18 +161,12 @@ void pinmux_set_io(enum pmux_pingrp pin, enum pmux_pin_io 
io)
assert(pmux_pingrp_isvalid(pin));
assert(pmux_pin_io_isvalid(io));
 
-   val = readl(reg);
-   if (io == PMUX_PIN_INPUT)
-   val |= (io  1)  IO_SHIFT;
-   else
-   val = ~(1  IO_SHIFT);
-   writel(val, reg);
+   update_field(reg, 1, IO_SHIFT, io == PMUX_PIN_INPUT);
 }
 
 static void pinmux_set_lock(enum pmux_pingrp pin, enum pmux_pin_lock lock)
 {
u32 *reg = REG(pin);
-   u32 val;
 
if (lock == PMUX_PIN_LOCK_DEFAULT)
return;
@@ -191,23 +175,18 @@ static void pinmux_set_lock(enum pmux_pingrp pin, enum 
pmux_pin_lock lock)
assert(pmux_pingrp_isvalid(pin));
assert(pmux_pin_lock_isvalid(lock));
 
-   val = readl(reg);
-   if (lock == PMUX_PIN_LOCK_ENABLE) {
-   val |= (1  LOCK_SHIFT);
-   } else {
+   if (lock == PMUX_PIN_LOCK_DISABLE) {
+   u32 val = readl(reg);
if (val  (1  LOCK_SHIFT))
printf(%s: Cannot clear LOCK bit!\n, __func__);
-   val = ~(1  LOCK_SHIFT);
}
-   writel(val, reg);
 
-   return;
+   update_field(reg, 1, LOCK_SHIFT, lock == PMUX_PIN_LOCK_ENABLE);
 }
 
 static void pinmux_set_od(enum pmux_pingrp pin, enum pmux_pin_od od)
 {
u32 *reg = REG(pin);
-   u32 val;
 
if (od == PMUX_PIN_OD_DEFAULT)
return;
@@ -216,21 +195,13 @@ static void pinmux_set_od(enum pmux_pingrp pin, enum 
pmux_pin_od od)
assert(pmux_pingrp_isvalid(pin));
assert(pmux_pin_od_isvalid(od));
 
-   val = readl(reg);
-   if (od == PMUX_PIN_OD_ENABLE)
-   val |= (1  OD_SHIFT);
-   else
-   val = ~(1  OD_SHIFT);
-   writel(val, reg);
-
-   return;
+   update_field(reg, 1, OD_SHIFT, od == PMUX_PIN_OD_ENABLE);
 }
 
 static void pinmux_set_ioreset(enum pmux_pingrp pin,
enum pmux_pin_ioreset ioreset)
 {
u32 *reg = REG(pin);
-   u32 val;
 
if (ioreset ==