[PATCH] pinctrl: rockchip: fix rk3288 gpio0 pull up configuration

[PATCH] pinctrl: rockchip: fix rk3288 gpio0 pull up configuration

[Sonny Rao]

On rk3288, for gpio bank 0, the registers which configure pull-ups
don't implement the enable bits in the upper half of the register,
unlike the other gpio configuration registers, and so the kernel
must perform a read-modify-write of the register to update a
particular gpio's pull up settings in that bank.

The current code is actually clobbering the contents of the register,
so this fixes it by using regmap_update_bits and masking out only the
bits which require updating.  In the case of gpio0 on rk3288 the upper
enable bits will just get ignored, and the other configurations won't
get clobbered.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
---
 drivers/pinctrl/pinctrl-rockchip.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index 5e8b2e0..bd00ae9 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -722,7 +722,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 	int reg, ret;
 	unsigned long flags;
 	u8 bit;
-	u32 data;
+	u32 data, mask;
 
 	dev_dbg(info->dev, "setting pull of GPIO%d-%d to %d\n",
 		 bank->bank_num, pin_num, pull);
@@ -751,6 +751,14 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 		/* enable the write to the equivalent lower bits */
 		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
 
+		/*
+		 * on rk3288, gpio bank 0 doesn't support the write enable, and
+		 * upper 16 bit are always zero, so we create a mask which will
+		 * only update the bits we want in the lower 16, while still
+		 * preserving write enable bits in upper 16.
+		 */
+		mask = data | (data >> 16);
+
 		switch (pull) {
 		case PIN_CONFIG_BIAS_DISABLE:
 			break;
@@ -770,7 +778,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 			return -EINVAL;
 		}
 
-		ret = regmap_write(regmap, reg, data);
+		ret = regmap_update_bits(regmap, reg, mask, data);
 
 		spin_unlock_irqrestore(&bank->slock, flags);
 		break;
-- 
1.8.3.2

[PATCH] pinctrl: rockchip: fix rk3288 gpio0 pull up configuration

[Heiko Stübner]

Hi Sonny,

Am Dienstag, 29. Juli 2014, 20:21:05 schrieb Sonny Rao:
> On rk3288, for gpio bank 0, the registers which configure pull-ups
> don't implement the enable bits in the upper half of the register,
> unlike the other gpio configuration registers, and so the kernel
> must perform a read-modify-write of the register to update a
> particular gpio's pull up settings in that bank.
> 
> The current code is actually clobbering the contents of the register,
> so this fixes it by using regmap_update_bits and masking out only the
> bits which require updating.  In the case of gpio0 on rk3288 the upper
> enable bits will just get ignored, and the other configurations won't
> get clobbered.

I have a similar fix, I had slightly forgotten about, in [0]. But I like
your approach a lot better, so could you expand your fix to also
handle the muxing and drive strength settings, which suffer from
the same handling difference on the rk3288?


Thanks
Heiko


[0] https://github.com/mmind/linux-rockchip/commit/c79d2a687c81b99fb566ef17b1c33ad45458e0be


> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index 5e8b2e0..bd00ae9 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -722,7 +722,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, int reg, ret;
>  	unsigned long flags;
>  	u8 bit;
> -	u32 data;
> +	u32 data, mask;
> 
>  	dev_dbg(info->dev, "setting pull of GPIO%d-%d to %d\n",
>  		 bank->bank_num, pin_num, pull);
> @@ -751,6 +751,14 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, /* enable the write to the equivalent lower bits */
>  		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
> 
> +		/*
> +		 * on rk3288, gpio bank 0 doesn't support the write enable, and
> +		 * upper 16 bit are always zero, so we create a mask which will
> +		 * only update the bits we want in the lower 16, while still
> +		 * preserving write enable bits in upper 16.
> +		 */
> +		mask = data | (data >> 16);
> +
>  		switch (pull) {
>  		case PIN_CONFIG_BIAS_DISABLE:
>  			break;
> @@ -770,7 +778,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, return -EINVAL;
>  		}
> 
> -		ret = regmap_write(regmap, reg, data);
> +		ret = regmap_update_bits(regmap, reg, mask, data);
> 
>  		spin_unlock_irqrestore(&bank->slock, flags);
>  		break;

[PATCH v2] pinctrl: rockchip: fix rk3288 gpio0 configuration

[Sonny Rao]

On rk3288, for gpio bank 0, the registers which configure pull-up,
iomux, and drive strength don't implement the enable bits in the upper
half of the register, unlike the other gpio configuration registers,
and so the kernel must perform a read-modify-write of the register to
update a particular gpio in that bank.

The current code is actually clobbering the contents of the register,
so this fixes it by using regmap_update_bits and masking out only the
bits which require updating.  In the case of bank0 on rk3288 the upper
enable bits will just get ignored, and the other configurations won't
get clobbered.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
---
v2: rebase onto latest pinctrl with drive strength and fix this bug on
  iomux and drive strength as well.
 
 drivers/pinctrl/pinctrl-rockchip.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index c15f7f9..4ff5dc3 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -438,7 +438,7 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	int reg, ret, mask;
 	unsigned long flags;
 	u8 bit;
-	u32 data;
+	u32 data, rmask;
 
 	if (iomux_num > 3)
 		return -EINVAL;
@@ -478,8 +478,9 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux)
 	spin_lock_irqsave(&bank->slock, flags);
 
 	data = (mask << (bit + 16));
+	rmask = data | (data >> 16);
 	data |= (mux & mask) << bit;
-	ret = regmap_write(regmap, reg, data);
+	ret = regmap_update_bits(regmap, reg, rmask, data);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
 
@@ -634,7 +635,7 @@ static int rk3288_set_drive(struct rockchip_pin_bank *bank, int pin_num,
 	struct regmap *regmap;
 	unsigned long flags;
 	int reg, ret, i;
-	u32 data;
+	u32 data, rmask;
 	u8 bit;
 
 	rk3288_calc_drv_reg_and_bit(bank, pin_num, &regmap, &reg, &bit);
@@ -657,9 +658,10 @@ static int rk3288_set_drive(struct rockchip_pin_bank *bank, int pin_num,
 
 	/* enable the write to the equivalent lower bits */
 	data = ((1 << RK3288_DRV_BITS_PER_PIN) - 1) << (bit + 16);
+	rmask = data | (data >> 16);
 	data |= (ret << bit);
 
-	ret = regmap_write(regmap, reg, data);
+	ret = regmap_update_bits(regmap, reg, rmask, data);
 	spin_unlock_irqrestore(&bank->slock, flags);
 
 	return ret;
@@ -722,7 +724,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 	int reg, ret;
 	unsigned long flags;
 	u8 bit;
-	u32 data;
+	u32 data, rmask;
 
 	dev_dbg(info->dev, "setting pull of GPIO%d-%d to %d\n",
 		 bank->bank_num, pin_num, pull);
@@ -750,6 +752,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 
 		/* enable the write to the equivalent lower bits */
 		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
+		rmask = data | (data >> 16);
 
 		switch (pull) {
 		case PIN_CONFIG_BIAS_DISABLE:
@@ -770,7 +773,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank,
 			return -EINVAL;
 		}
 
-		ret = regmap_write(regmap, reg, data);
+		ret = regmap_update_bits(regmap, reg, rmask, data);
 
 		spin_unlock_irqrestore(&bank->slock, flags);
 		break;
-- 
1.8.3.2

[PATCH v2] pinctrl: rockchip: fix rk3288 gpio0 configuration

[Heiko Stübner]

Am Donnerstag, 31. Juli 2014, 22:58:00 schrieb Sonny Rao:
> On rk3288, for gpio bank 0, the registers which configure pull-up,
> iomux, and drive strength don't implement the enable bits in the upper
> half of the register, unlike the other gpio configuration registers,
> and so the kernel must perform a read-modify-write of the register to
> update a particular gpio in that bank.
> 
> The current code is actually clobbering the contents of the register,
> so this fixes it by using regmap_update_bits and masking out only the
> bits which require updating.  In the case of bank0 on rk3288 the upper
> enable bits will just get ignored, and the other configurations won't
> get clobbered.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>


thanks for this fix
Heiko

> ---
> v2: rebase onto latest pinctrl with drive strength and fix this bug on
>   iomux and drive strength as well.
> 
>  drivers/pinctrl/pinctrl-rockchip.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index c15f7f9..4ff5dc3 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -438,7 +438,7 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) int reg, ret, mask;
>  	unsigned long flags;
>  	u8 bit;
> -	u32 data;
> +	u32 data, rmask;
> 
>  	if (iomux_num > 3)
>  		return -EINVAL;
> @@ -478,8 +478,9 @@ static int rockchip_set_mux(struct rockchip_pin_bank
> *bank, int pin, int mux) spin_lock_irqsave(&bank->slock, flags);
> 
>  	data = (mask << (bit + 16));
> +	rmask = data | (data >> 16);
>  	data |= (mux & mask) << bit;
> -	ret = regmap_write(regmap, reg, data);
> +	ret = regmap_update_bits(regmap, reg, rmask, data);
> 
>  	spin_unlock_irqrestore(&bank->slock, flags);
> 
> @@ -634,7 +635,7 @@ static int rk3288_set_drive(struct rockchip_pin_bank
> *bank, int pin_num, struct regmap *regmap;
>  	unsigned long flags;
>  	int reg, ret, i;
> -	u32 data;
> +	u32 data, rmask;
>  	u8 bit;
> 
>  	rk3288_calc_drv_reg_and_bit(bank, pin_num, &regmap, &reg, &bit);
> @@ -657,9 +658,10 @@ static int rk3288_set_drive(struct rockchip_pin_bank
> *bank, int pin_num,
> 
>  	/* enable the write to the equivalent lower bits */
>  	data = ((1 << RK3288_DRV_BITS_PER_PIN) - 1) << (bit + 16);
> +	rmask = data | (data >> 16);
>  	data |= (ret << bit);
> 
> -	ret = regmap_write(regmap, reg, data);
> +	ret = regmap_update_bits(regmap, reg, rmask, data);
>  	spin_unlock_irqrestore(&bank->slock, flags);
> 
>  	return ret;
> @@ -722,7 +724,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, int reg, ret;
>  	unsigned long flags;
>  	u8 bit;
> -	u32 data;
> +	u32 data, rmask;
> 
>  	dev_dbg(info->dev, "setting pull of GPIO%d-%d to %d\n",
>  		 bank->bank_num, pin_num, pull);
> @@ -750,6 +752,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank,
> 
>  		/* enable the write to the equivalent lower bits */
>  		data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16);
> +		rmask = data | (data >> 16);
> 
>  		switch (pull) {
>  		case PIN_CONFIG_BIAS_DISABLE:
> @@ -770,7 +773,7 @@ static int rockchip_set_pull(struct rockchip_pin_bank
> *bank, return -EINVAL;
>  		}
> 
> -		ret = regmap_write(regmap, reg, data);
> +		ret = regmap_update_bits(regmap, reg, rmask, data);
> 
>  		spin_unlock_irqrestore(&bank->slock, flags);
>  		break;

[PATCH v2] pinctrl: rockchip: fix rk3288 gpio0 configuration

[Doug Anderson]

Sonny,

On Thu, Jul 31, 2014 at 10:58 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> On rk3288, for gpio bank 0, the registers which configure pull-up,
> iomux, and drive strength don't implement the enable bits in the upper
> half of the register, unlike the other gpio configuration registers,
> and so the kernel must perform a read-modify-write of the register to
> update a particular gpio in that bank.
>
> The current code is actually clobbering the contents of the register,
> so this fixes it by using regmap_update_bits and masking out only the
> bits which require updating.  In the case of bank0 on rk3288 the upper
> enable bits will just get ignored, and the other configurations won't
> get clobbered.
>
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
> v2: rebase onto latest pinctrl with drive strength and fix this bug on
>   iomux and drive strength as well.
>
>  drivers/pinctrl/pinctrl-rockchip.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH] pinctrl: rockchip: fix rk3288 gpio0 pull up configuration

[Linus Walleij]

On Wed, Jul 30, 2014 at 5:21 AM, Sonny Rao <sonnyrao@chromium.org> wrote:

> On rk3288, for gpio bank 0, the registers which configure pull-ups
> don't implement the enable bits in the upper half of the register,
> unlike the other gpio configuration registers, and so the kernel
> must perform a read-modify-write of the register to update a
> particular gpio's pull up settings in that bank.
>
> The current code is actually clobbering the contents of the register,
> so this fixes it by using regmap_update_bits and masking out only the
> bits which require updating.  In the case of gpio0 on rk3288 the upper
> enable bits will just get ignored, and the other configurations won't
> get clobbered.
>
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>

Waiting for a v2 fixing the other issues pointed out by Heiko.

Yours,
Linus Walleij

[PATCH] pinctrl: rockchip: fix rk3288 gpio0 pull up configuration

[Heiko Stübner]

Am Freitag, 8. August 2014, 15:37:46 schrieb Linus Walleij:
> On Wed, Jul 30, 2014 at 5:21 AM, Sonny Rao <sonnyrao@chromium.org> wrote:
> > On rk3288, for gpio bank 0, the registers which configure pull-ups
> > don't implement the enable bits in the upper half of the register,
> > unlike the other gpio configuration registers, and so the kernel
> > must perform a read-modify-write of the register to update a
> > particular gpio's pull up settings in that bank.
> > 
> > The current code is actually clobbering the contents of the register,
> > so this fixes it by using regmap_update_bits and masking out only the
> > bits which require updating.  In the case of gpio0 on rk3288 the upper
> > enable bits will just get ignored, and the other configurations won't
> > get clobbered.
> > 
> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> 
> Waiting for a v2 fixing the other issues pointed out by Heiko.

Sonny posted v2 on 2014-08-01 as reply to my comment, but it might still be 
sitting in your inbox-backlog :-)

Checking Sonny's v2-mail again I see that you did somehow land in the Cc-list 
and not the To-list, which might be the reason you didn't see it yet?


Heiko

[PATCH] pinctrl: rockchip: fix rk3288 gpio0 pull up configuration

[Sonny Rao]

On Fri, Aug 8, 2014 at 7:09 AM, Heiko St?bner <heiko@sntech.de> wrote:
> Am Freitag, 8. August 2014, 15:37:46 schrieb Linus Walleij:
>> On Wed, Jul 30, 2014 at 5:21 AM, Sonny Rao <sonnyrao@chromium.org> wrote:
>> > On rk3288, for gpio bank 0, the registers which configure pull-ups
>> > don't implement the enable bits in the upper half of the register,
>> > unlike the other gpio configuration registers, and so the kernel
>> > must perform a read-modify-write of the register to update a
>> > particular gpio's pull up settings in that bank.
>> >
>> > The current code is actually clobbering the contents of the register,
>> > so this fixes it by using regmap_update_bits and masking out only the
>> > bits which require updating.  In the case of gpio0 on rk3288 the upper
>> > enable bits will just get ignored, and the other configurations won't
>> > get clobbered.
>> >
>> > Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>
>> Waiting for a v2 fixing the other issues pointed out by Heiko.
>
> Sonny posted v2 on 2014-08-01 as reply to my comment, but it might still be
> sitting in your inbox-backlog :-)
>
> Checking Sonny's v2-mail again I see that you did somehow land in the Cc-list
> and not the To-list, which might be the reason you didn't see it yet?

Yeah, let me know if I'm doing something incorrect with respect to procedure.
I will also re-send to Linus if he needs.

Thanks,
Sonny

>
>
> Heiko

[PATCH v2] pinctrl: rockchip: fix rk3288 gpio0 configuration

[Linus Walleij]

On Fri, Aug 1, 2014 at 7:58 AM, Sonny Rao <sonnyrao@chromium.org> wrote:

> On rk3288, for gpio bank 0, the registers which configure pull-up,
> iomux, and drive strength don't implement the enable bits in the upper
> half of the register, unlike the other gpio configuration registers,
> and so the kernel must perform a read-modify-write of the register to
> update a particular gpio in that bank.
>
> The current code is actually clobbering the contents of the register,
> so this fixes it by using regmap_update_bits and masking out only the
> bits which require updating.  In the case of bank0 on rk3288 the upper
> enable bits will just get ignored, and the other configurations won't
> get clobbered.
>
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
> v2: rebase onto latest pinctrl with drive strength and fix this bug on
>   iomux and drive strength as well.

Patch applied with Heikos and Dougs review tags.

Thanks!
Linus Walleij