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

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

[Stephen Warren]

From: Stephen Warren <swarren@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 <swarren@nvidia.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
V3: Rename update_field() to update_reg_mask_shift_val() to make the
    parameter order more obvious.
V2: New patch.

(I'm only reposting V3 of this one patch in order to avoid spamming the
list with the other huge table replacements in the series. Hopefully this
isn't too painful when applying them).
---
 arch/arm/cpu/tegra-common/pinmux-common.c | 140 ++++++------------------------
 1 file changed, 28 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..7d5c74055644 100644
--- a/arch/arm/cpu/tegra-common/pinmux-common.c
+++ b/arch/arm/cpu/tegra-common/pinmux-common.c
@@ -87,11 +87,16 @@
 #define IO_RESET_SHIFT	8
 #define RCV_SEL_SHIFT	9
 
+static inline void update_reg_mask_shift_val(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 +115,30 @@ 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_reg_mask_shift_val(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_reg_mask_shift_val(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_reg_mask_shift_val(reg, 1, TRI_SHIFT(pin),
+				  tri == PMUX_TRI_TRISTATE);
 }
 
 void pinmux_tristate_enable(enum pmux_pingrp pin)
@@ -162,7 +155,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 +163,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_reg_mask_shift_val(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 +177,19 @@ 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_reg_mask_shift_val(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 +198,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_reg_mask_shift_val(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 == PMUX_PIN_IO_RESET_DEFAULT)
 		return;
@@ -239,14 +213,8 @@ static void pinmux_set_ioreset(enum pmux_pingrp pin,
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_ioreset_isvalid(ioreset));
 
-	val = readl(reg);
-	if (ioreset == PMUX_PIN_IO_RESET_ENABLE)
-		val |= (1 << IO_RESET_SHIFT);
-	else
-		val &= ~(1 << IO_RESET_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, IO_RESET_SHIFT,
+				  ioreset == PMUX_PIN_IO_RESET_ENABLE);
 }
 
 #ifdef TEGRA_PMX_HAS_RCV_SEL
@@ -254,7 +222,6 @@ static void pinmux_set_rcv_sel(enum pmux_pingrp pin,
 				enum pmux_pin_rcv_sel rcv_sel)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (rcv_sel == PMUX_PIN_RCV_SEL_DEFAULT)
 		return;
@@ -263,14 +230,8 @@ static void pinmux_set_rcv_sel(enum pmux_pingrp pin,
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_rcv_sel_isvalid(rcv_sel));
 
-	val = readl(reg);
-	if (rcv_sel == PMUX_PIN_RCV_SEL_HIGH)
-		val |= (1 << RCV_SEL_SHIFT);
-	else
-		val &= ~(1 << RCV_SEL_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, RCV_SEL_SHIFT,
+				  rcv_sel == PMUX_PIN_RCV_SEL_HIGH);
 }
 #endif /* TEGRA_PMX_HAS_RCV_SEL */
 #endif /* TEGRA_PMX_HAS_PIN_IO_BIT_ETC */
@@ -337,7 +298,6 @@ void pinmux_config_pingrp_table(const struct pmux_pingrp_config *config,
 static void pinmux_set_drvup_slwf(enum pmux_drvgrp grp, int slwf)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (slwf == PMUX_SLWF_NONE)
@@ -347,18 +307,12 @@ static void pinmux_set_drvup_slwf(enum pmux_drvgrp grp, int slwf)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_slw_isvalid(slwf));
 
-	val = readl(reg);
-	val &= ~SLWF_MASK;
-	val |= (slwf << SLWF_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, SLWF_MASK, SLWF_SHIFT, slwf);
 }
 
 static void pinmux_set_drvdn_slwr(enum pmux_drvgrp grp, int slwr)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (slwr == PMUX_SLWR_NONE)
@@ -368,18 +322,12 @@ static void pinmux_set_drvdn_slwr(enum pmux_drvgrp grp, int slwr)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_slw_isvalid(slwr));
 
-	val = readl(reg);
-	val &= ~SLWR_MASK;
-	val |= (slwr << SLWR_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, SLWR_MASK, SLWR_SHIFT, slwr);
 }
 
 static void pinmux_set_drvup(enum pmux_drvgrp grp, int drvup)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (drvup == PMUX_DRVUP_NONE)
@@ -389,18 +337,12 @@ static void pinmux_set_drvup(enum pmux_drvgrp grp, int drvup)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_drv_isvalid(drvup));
 
-	val = readl(reg);
-	val &= ~DRVUP_MASK;
-	val |= (drvup << DRVUP_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, DRVUP_MASK, DRVUP_SHIFT, drvup);
 }
 
 static void pinmux_set_drvdn(enum pmux_drvgrp grp, int drvdn)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (drvdn == PMUX_DRVDN_NONE)
@@ -410,18 +352,12 @@ static void pinmux_set_drvdn(enum pmux_drvgrp grp, int drvdn)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_drv_isvalid(drvdn));
 
-	val = readl(reg);
-	val &= ~DRVDN_MASK;
-	val |= (drvdn << DRVDN_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, DRVDN_MASK, DRVDN_SHIFT, drvdn);
 }
 
 static void pinmux_set_lpmd(enum pmux_drvgrp grp, enum pmux_lpmd lpmd)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (lpmd == PMUX_LPMD_NONE)
@@ -431,18 +367,12 @@ static void pinmux_set_lpmd(enum pmux_drvgrp grp, enum pmux_lpmd lpmd)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_lpmd_isvalid(lpmd));
 
-	val = readl(reg);
-	val &= ~LPMD_MASK;
-	val |= (lpmd << LPMD_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, LPMD_MASK, LPMD_SHIFT, lpmd);
 }
 
 static void pinmux_set_schmt(enum pmux_drvgrp grp, enum pmux_schmt schmt)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (schmt == PMUX_SCHMT_NONE)
@@ -452,20 +382,13 @@ static void pinmux_set_schmt(enum pmux_drvgrp grp, enum pmux_schmt schmt)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_schmt_isvalid(schmt));
 
-	val = readl(reg);
-	if (schmt == PMUX_SCHMT_ENABLE)
-		val |= (1 << SCHMT_SHIFT);
-	else
-		val &= ~(1 << SCHMT_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, SCHMT_SHIFT,
+				  schmt == PMUX_SCHMT_ENABLE);
 }
 
 static void pinmux_set_hsm(enum pmux_drvgrp grp, enum pmux_hsm hsm)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (hsm == PMUX_HSM_NONE)
@@ -475,14 +398,7 @@ static void pinmux_set_hsm(enum pmux_drvgrp grp, enum pmux_hsm hsm)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_hsm_isvalid(hsm));
 
-	val = readl(reg);
-	if (hsm == PMUX_HSM_ENABLE)
-		val |= (1 << HSM_SHIFT);
-	else
-		val &= ~(1 << HSM_SHIFT);
-	writel(val, reg);
-
-	return;
+	update_reg_mask_shift_val(reg, 1, HSM_SHIFT, hsm == PMUX_HSM_ENABLE);
 }
 
 static void pinmux_config_drvgrp(const struct pmux_drvgrp_config *config)
-- 
1.8.1.5

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

[Wolfgang Denk]

Dear Stephen Warren,

In message <1395764855-23377-1-git-send-email-swarren@wwwdotorg.org> you wrote:
> 
> +static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
> +					     u32 val)
> +{
> +	clrsetbits_le32(reg, mask << shift, val << shift);
> +}

No, please do not do that.  Please use plain clrsetbits_le32() as is.
All these hidden shifts are (a) mostly unreadable and (b) sometimes
dangerous.

Thanks.

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 at denx.de
The use of COBOL cripples the mind; its teaching  should,  therefore,
be regarded as a criminal offence.
          -- Edsger W. Dijkstra, SIGPLAN Notices, Volume 17, Number 5

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

[Stephen Warren]

On 03/25/2014 10:54 AM, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <1395764855-23377-1-git-send-email-swarren@wwwdotorg.org> you wrote:
>>
>> +static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
>> +					     u32 val)
>> +{
>> +	clrsetbits_le32(reg, mask << shift, val << shift);
>> +}
> 
> No, please do not do that.  Please use plain clrsetbits_le32() as is.
> All these hidden shifts are (a) mostly unreadable and (b) sometimes
> dangerous.

Seriously, are you joking now?????

If I was to write out the clrsetbits_le32() at each call site, I'd be
writing out this supposedly dangerous shift N times instead of once. If
the shift is somehow dangerous (BTW, it isn't!) then surely isolating it
in one place, so that mistakes aren't made when writing the duplicate
copies, is the right thing to do.

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

[Wolfgang Denk]

Dear Stephen Warren,

In message <5331B55B.7080209@wwwdotorg.org> you wrote:
>
> > No, please do not do that.  Please use plain clrsetbits_le32() as is.
> > All these hidden shifts are (a) mostly unreadable and (b) sometimes
> > dangerous.
> 
> Seriously, are you joking now?????

No, I am not.

> If I was to write out the clrsetbits_le32() at each call site, I'd be
> writing out this supposedly dangerous shift N times instead of once. If

N = 2, to be precise.  And you have to type it only once, but to
maintain that code for a long, long time.

> the shift is somehow dangerous (BTW, it isn't!) then surely isolating it
> in one place, so that mistakes aren't made when writing the duplicate
> copies, is the right thing to do.

Well, I've just fixed a number of places were such code _was_
dangerous, but well hidden under a nice wrapper function like yours.

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 at denx.de
Sometimes, too long is too long.                          - Joe Crowe

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

[Wolfgang Denk]

From: Stephen Warren <swarren@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 <swarren@nvidia.com>
Signed-off-by: Wolfgang Denk <wd@denx.de>
---
V4: [wd] Drop update_reg_mask_shift_val() and use clrsetbits_le32()
    directly.
V3: Rename update_field() to update_reg_mask_shift_val() to make the
    parameter order more obvious.
V2: New patch.

 arch/arm/cpu/tegra-common/pinmux-common.c | 137 ++++++------------------------
 1 file changed, 25 insertions(+), 112 deletions(-)

diff --git a/arch/arm/cpu/tegra-common/pinmux-common.c b/arch/arm/cpu/tegra-common/pinmux-common.c
index 32a46d5..fec5e37 100644
--- a/arch/arm/cpu/tegra-common/pinmux-common.c
+++ b/arch/arm/cpu/tegra-common/pinmux-common.c
@@ -91,7 +91,6 @@ 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 +109,30 @@ 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);
+	clrsetbits_le32(reg, 3 << MUX_SHIFT(pin), mux << MUX_SHIFT(pin));
 }
 
 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);
+	clrsetbits_le32(reg, 3 << PULL_SHIFT(pin), pupd << PULL_SHIFT(pin));
 }
 
 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);
+	clrsetbits_le32(reg, 1 << TRI_SHIFT(pin),
+			(tri == PMUX_TRI_TRISTATE) << TRI_SHIFT(pin));
 }
 
 void pinmux_tristate_enable(enum pmux_pingrp pin)
@@ -162,7 +149,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 +157,13 @@ 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);
+	clrsetbits_le32(reg, 1 << IO_SHIFT,
+			(io == PMUX_PIN_INPUT) << IO_SHIFT);
 }
 
 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 +172,19 @@ 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;
+	clrsetbits_le32(reg, 1 << LOCK_SHIFT,
+			(lock == PMUX_PIN_LOCK_ENABLE) << LOCK_SHIFT);
 }
 
 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 +193,14 @@ 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;
+	clrsetbits_le32(reg, 1 << OD_SHIFT,
+			(od == PMUX_PIN_OD_ENABLE) << OD_SHIFT);
 }
 
 static void pinmux_set_ioreset(enum pmux_pingrp pin,
 				enum pmux_pin_ioreset ioreset)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (ioreset == PMUX_PIN_IO_RESET_DEFAULT)
 		return;
@@ -239,14 +209,8 @@ static void pinmux_set_ioreset(enum pmux_pingrp pin,
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_ioreset_isvalid(ioreset));
 
-	val = readl(reg);
-	if (ioreset == PMUX_PIN_IO_RESET_ENABLE)
-		val |= (1 << IO_RESET_SHIFT);
-	else
-		val &= ~(1 << IO_RESET_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << IO_RESET_SHIFT,
+		(ioreset == PMUX_PIN_IO_RESET_ENABLE) << IO_RESET_SHIFT);
 }
 
 #ifdef TEGRA_PMX_HAS_RCV_SEL
@@ -254,7 +218,6 @@ static void pinmux_set_rcv_sel(enum pmux_pingrp pin,
 				enum pmux_pin_rcv_sel rcv_sel)
 {
 	u32 *reg = REG(pin);
-	u32 val;
 
 	if (rcv_sel == PMUX_PIN_RCV_SEL_DEFAULT)
 		return;
@@ -263,14 +226,8 @@ static void pinmux_set_rcv_sel(enum pmux_pingrp pin,
 	assert(pmux_pingrp_isvalid(pin));
 	assert(pmux_pin_rcv_sel_isvalid(rcv_sel));
 
-	val = readl(reg);
-	if (rcv_sel == PMUX_PIN_RCV_SEL_HIGH)
-		val |= (1 << RCV_SEL_SHIFT);
-	else
-		val &= ~(1 << RCV_SEL_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << RCV_SEL_SHIFT,
+		(rcv_sel == PMUX_PIN_RCV_SEL_HIGH) << RCV_SEL_SHIFT);
 }
 #endif /* TEGRA_PMX_HAS_RCV_SEL */
 #endif /* TEGRA_PMX_HAS_PIN_IO_BIT_ETC */
@@ -337,7 +294,6 @@ void pinmux_config_pingrp_table(const struct pmux_pingrp_config *config,
 static void pinmux_set_drvup_slwf(enum pmux_drvgrp grp, int slwf)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (slwf == PMUX_SLWF_NONE)
@@ -347,18 +303,12 @@ static void pinmux_set_drvup_slwf(enum pmux_drvgrp grp, int slwf)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_slw_isvalid(slwf));
 
-	val = readl(reg);
-	val &= ~SLWF_MASK;
-	val |= (slwf << SLWF_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, SLWF_MASK << SLWF_SHIFT, slwf << SLWF_SHIFT);
 }
 
 static void pinmux_set_drvdn_slwr(enum pmux_drvgrp grp, int slwr)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (slwr == PMUX_SLWR_NONE)
@@ -368,18 +318,12 @@ static void pinmux_set_drvdn_slwr(enum pmux_drvgrp grp, int slwr)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_slw_isvalid(slwr));
 
-	val = readl(reg);
-	val &= ~SLWR_MASK;
-	val |= (slwr << SLWR_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, SLWR_MASK << SLWR_SHIFT, slwr << SLWR_SHIFT);
 }
 
 static void pinmux_set_drvup(enum pmux_drvgrp grp, int drvup)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (drvup == PMUX_DRVUP_NONE)
@@ -389,18 +333,12 @@ static void pinmux_set_drvup(enum pmux_drvgrp grp, int drvup)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_drv_isvalid(drvup));
 
-	val = readl(reg);
-	val &= ~DRVUP_MASK;
-	val |= (drvup << DRVUP_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, DRVUP_MASK << DRVUP_SHIFT, drvup << DRVUP_SHIFT);
 }
 
 static void pinmux_set_drvdn(enum pmux_drvgrp grp, int drvdn)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (drvdn == PMUX_DRVDN_NONE)
@@ -410,18 +348,12 @@ static void pinmux_set_drvdn(enum pmux_drvgrp grp, int drvdn)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_drv_isvalid(drvdn));
 
-	val = readl(reg);
-	val &= ~DRVDN_MASK;
-	val |= (drvdn << DRVDN_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, DRVDN_MASK << DRVDN_SHIFT, drvdn << DRVDN_SHIFT);
 }
 
 static void pinmux_set_lpmd(enum pmux_drvgrp grp, enum pmux_lpmd lpmd)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (lpmd == PMUX_LPMD_NONE)
@@ -431,18 +363,12 @@ static void pinmux_set_lpmd(enum pmux_drvgrp grp, enum pmux_lpmd lpmd)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_lpmd_isvalid(lpmd));
 
-	val = readl(reg);
-	val &= ~LPMD_MASK;
-	val |= (lpmd << LPMD_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, LPMD_MASK << LPMD_SHIFT, lpmd << LPMD_SHIFT);
 }
 
 static void pinmux_set_schmt(enum pmux_drvgrp grp, enum pmux_schmt schmt)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (schmt == PMUX_SCHMT_NONE)
@@ -452,20 +378,13 @@ static void pinmux_set_schmt(enum pmux_drvgrp grp, enum pmux_schmt schmt)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_schmt_isvalid(schmt));
 
-	val = readl(reg);
-	if (schmt == PMUX_SCHMT_ENABLE)
-		val |= (1 << SCHMT_SHIFT);
-	else
-		val &= ~(1 << SCHMT_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << SCHMT_SHIFT,
+			(schmt == PMUX_SCHMT_ENABLE) << SCHMT_SHIFT);
 }
 
 static void pinmux_set_hsm(enum pmux_drvgrp grp, enum pmux_hsm hsm)
 {
 	u32 *reg = DRV_REG(grp);
-	u32 val;
 
 	/* NONE means unspecified/do not change/use POR value */
 	if (hsm == PMUX_HSM_NONE)
@@ -475,14 +394,8 @@ static void pinmux_set_hsm(enum pmux_drvgrp grp, enum pmux_hsm hsm)
 	assert(pmux_drvgrp_isvalid(grp));
 	assert(pmux_hsm_isvalid(hsm));
 
-	val = readl(reg);
-	if (hsm == PMUX_HSM_ENABLE)
-		val |= (1 << HSM_SHIFT);
-	else
-		val &= ~(1 << HSM_SHIFT);
-	writel(val, reg);
-
-	return;
+	clrsetbits_le32(reg, 1 << HSM_SHIFT,
+			(hsm == PMUX_HSM_ENABLE) << HSM_SHIFT);
 }
 
 static void pinmux_config_drvgrp(const struct pmux_drvgrp_config *config)
-- 
1.8.5.3

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

[Stephen Warren]

On 03/25/2014 01:09 PM, Wolfgang Denk wrote:
> From: Stephen Warren <swarren@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 <swarren@nvidia.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> ---
> V4: [wd] Drop update_reg_mask_shift_val() and use clrsetbits_le32()
>     directly.

I believe your own argument compels you to NAK this patch yourself.

In both V3 and V4:

1) Some very simple and obviously understandable open-coded bit
manipulation is replaced with a function call which hides the details of
the bit manipulation.

2) In both cases, the order of parameters to the function does actually
appear to be obvious from the function name: clrsetbits_le32(clr, set),
update_reg_mask_shift_val(reg, mask, shift, val)

3) I've seen plenty of examples where the function name doesn't
correctly describe what the function does, or the parameter order
doesn't match what would appear to be logical. Hence, in neither case is
point (2) relevant; one must /always/ check or remember the prototype of
a function in order to validate that the parameters at a call-site are
correct. Not checking means making an assumption, and assumptions can be
incorrect.

Now, I believe you argued that it was unsafe to hide the details of the
bit manipulation behind a function precisely because it was then harder
to see what the code was doing, and easier to pass the wrong values to
the function parameters, and this wouldn't be obvious at the call site.

I believe the /exact/ same argument applies no matter whether the
function is clrsetbits_le32() or update_reg_mask_shift_val().

Hence, if V3 which used update_reg_mask_shift_val() was unacceptable,
then so is this.

So NAK.

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

[Tom Rini]

On Tue, Mar 25, 2014 at 05:54:10PM +0100, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <1395764855-23377-1-git-send-email-swarren@wwwdotorg.org> you wrote:
> > 
> > +static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
> > +					     u32 val)
> > +{
> > +	clrsetbits_le32(reg, mask << shift, val << shift);
> > +}
> 
> No, please do not do that.  Please use plain clrsetbits_le32() as is.
> All these hidden shifts are (a) mostly unreadable and (b) sometimes
> dangerous.

No, this is why the lack of comments hurts things.  This isn't sr32 from
OMAP-land (which was on my todo list somewhere, thanks).  sr32 was an
incorrect generic function.  This is a specific-use function that should
say something like:
/*
 * Set the correct pinmux value for a given part.  We need to clear out
 * M bits worth of the field and then set possibly less than M bits
 * worth of value.
 */

With respect to danger / readability, no, either way is just as
dangerous (or not dangerous) and it's still fairly dense code either
way and fixing a problem with an incorrect shift value is the same
effort.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140325/95b3eb9b/attachment.pgp>

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

[Tom Rini]

On Tue, Mar 25, 2014 at 10:27:35AM -0600, Stephen Warren wrote:

> From: Stephen Warren <swarren@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.

Since things got a bit heated here while I was reading some other
stuff...

[snip]

> +static inline void update_reg_mask_shift_val(u32 *reg, u32 mask, u32 shift,
> +					     u32 val)
> +{
> +	clrsetbits_le32(reg, mask << shift, val << shift);
> +}

So, lack of comments bad.  Intention, good.  We have a bitfield of size
M (that's all cleared in the mask) and value that may be less than M
bits wide.  The name is a mouthful (but I see where Simon was coming
from, had I caught in time I might have suggested a comment instead.

But as Wolfgang's v4 shows, it's also not hard to just call
clrsetbits_le32 directly.  Arguably the cases where mask==1 we should
just call setbits_le32 but that's not a big deal.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140325/f5fbda1d/attachment.pgp>

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

[Wolfgang Denk]

Dear Tom,

In message <20140325200420.GU16360@bill-the-cat> you wrote:
> 
> With respect to danger / readability, no, either way is just as
> dangerous (or not dangerous) and it's still fairly dense code either
> way and fixing a problem with an incorrect shift value is the same
> effort.

The key problem which I detected with sr32() was that it was in
several places called with a width of 32 - which looked perfectly fine
when the intention was to clear / set the whole 32 bit variable.  This
went unnoticed becuase it was just a normally looking argument.  If
the shift operation that resulted from that had been visible, the
problem would have been much easier to detect.  Seeing an expression
"value << 32" on u32 data types rings some alarms.

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 I had to live my life again,  I'd  make  the  same  mistakes, only
sooner.                                          -- Tallulah Bankhead

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

[Wolfgang Denk]

Dear Tom,

In message <20140325200435.GV16360@bill-the-cat> you wrote:
> 
> But as Wolfgang's v4 shows, it's also not hard to just call
> clrsetbits_le32 directly.  Arguably the cases where mask==1 we should
> just call setbits_le32 but that's not a big deal.

We would have to call setbits_le32() or clrbits_le32() depending on
the arguments...

I was already about to rewrite the code more in the style previously
used, i. e. turn for example

134         clrsetbits_le32(reg, 1 << TRI_SHIFT(pin),
135                         (tri == PMUX_TRI_TRISTATE) << TRI_SHIFT(pin));

back into

	if (tri == PMUX_TRI_TRISTATE)
		setbits_le32(reg, 1 << TRI_SHIFT(pin));
	else
		clrbits_le32(reg, 1 << TRI_SHIFT(pin));

but then I decided to keep the changes copmpared to the
update_reg_mask_shift_val() version minimal.

If the if/then version should be preferred, I can easily redo that
patch.

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 at denx.de
No, I'm not going to explain it. If you  can't  figure  it  out,  you
didn't want to know anyway... :-)
                   - Larry Wall in <1991Aug7.180856.2854@netlabs.com>