diff mbox

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

Message ID 1395764855-23377-1-git-send-email-swarren@wwwdotorg.org
State Superseded
Delegated to: Tom Warren
Headers show

Commit Message

Stephen Warren March 25, 2014, 4:27 p.m. UTC 
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(-)

Comments

Wolfgang Denk March 25, 2014, 4:54 p.m. UTC | #1 
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
Stephen Warren March 25, 2014, 4:56 p.m. UTC | #2 
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.
Wolfgang Denk March 25, 2014, 5:06 p.m. UTC | #3 
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
Tom Rini March 25, 2014, 8:04 p.m. UTC | #4 
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 Rini March 25, 2014, 8:04 p.m. UTC | #5 
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.
Wolfgang Denk March 25, 2014, 10:19 p.m. UTC | #6 
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
Wolfgang Denk March 25, 2014, 10:26 p.m. UTC | #7 
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
diff mbox

Patch

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)