Re: [U-Boot] [PATCH] pinctrl: rockchip: Add 32bit writing function for rk3288 gpio0 pinctrl

[David Wu]

Hi Philipp,

在 2019/3/29 下午10:40, Philipp Tomsich 写道:

David,

I am applying this one as a last-minute fix for 2019.4.
However, I’ll still need the v2 for the next cycle, as I my review 
comments still apply.




Sorry, there was a lot of things in March.
I think I could push v2 at this week.


Thanks,
Philipp.

On 12.02.2019, at 12:55, Philipp Tomsich 
> wrote:




On 12.02.2019, at 12:51, David Wu > wrote:


There are no higher 16 writing corresponding bits for pmu_gpio0's
iomux/drive/pull at rk3288, need to read the value from register
firstly. Add the flag to distinguish it from normal registers.

Signed-off-by: David Wu >

---
drivers/pinctrl/rockchip/pinctrl-rk3288.c | 17 ++--
.../pinctrl/rockchip/pinctrl-rockchip-core.c  | 39 ++-
drivers/pinctrl/rockchip/pinctrl-rockchip.h   | 33 
3 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c 
b/drivers/pinctrl/rockchip/pinctrl-rk3288.c

index 60585f3208..8b6ce11a63 100644
--- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c
+++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
@@ -92,10 +92,19 @@ static void rk3288_calc_drv_reg_and_bit(struct 
rockchip_pin_bank *bank,

}

static struct rockchip_pin_bank rk3288_pin_banks[] = {
-PIN_BANK_IOMUX_FLAGS(0, 24, "gpio0", IOMUX_SOURCE_PMU,
-IOMUX_SOURCE_PMU,
-IOMUX_SOURCE_PMU,
-IOMUX_UNROUTED
+PIN_BANK_IOMUX_DRV_PULL_FLAGS(0, 24, "gpio0",
+ IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
+ IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
+ IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
+ IOMUX_UNROUTED,
+ DRV_TYPE_WRITABLE_32BIT,
+ DRV_TYPE_WRITABLE_32BIT,
+ DRV_TYPE_WRITABLE_32BIT,
+ 0,
+ PULL_TYPE_WRITABLE_32BIT,
+ PULL_TYPE_WRITABLE_32BIT,
+ PULL_TYPE_WRITABLE_32BIT,
+ 0
   ),
PIN_BANK_IOMUX_FLAGS(1, 32, "gpio1", IOMUX_UNROUTED,
IOMUX_UNROUTED,
diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c 
b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c

index b84b079064..ce935656f0 100644
--- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
+++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
@@ -228,7 +228,13 @@ static int rockchip_set_mux(struct 
rockchip_pin_bank *bank, int pin, int mux)

}
}

-data = (mask << (bit + 16));
+if (mux_type & IOMUX_WRITABLE_32BIT) {
+regmap_read(regmap, reg, );
+data &= ~(mask << bit);
+} else {
+data = (mask << (bit + 16));
+}
+


This can not be optimised out by the compiler (for all the other 
platforms that don’t need it).
Please structure this (and the entire driver) to not pull in unneeded 
baggage that is only used

by one or a few devices.


data |= (mux & mask) << bit;
ret = regmap_write(regmap, reg, data);

@@ -252,7 +258,8 @@ static int rockchip_set_drive_perpin(struct 
rockchip_pin_bank *bank,

int reg, ret, i;
u32 data, rmask_bits, temp;
u8 bit;
-int drv_type = bank->drv[pin_num / 8].drv_type;
+/* Where need to clean the special mask for rockchip_perpin_drv_list */
+int drv_type = bank->drv[pin_num / 8].drv_type & (~DRV_TYPE_IO_MASK);

debug("setting drive of GPIO%d-%d to %d\n", bank->bank_num,
 pin_num, strength);
@@ -324,10 +331,15 @@ static int rockchip_set_drive_perpin(struct 
rockchip_pin_bank *bank,

return -EINVAL;
}

-/* enable the write to the equivalent lower bits */
-data = ((1 << rmask_bits) - 1) << (bit + 16);
-data |= (ret << bit);
+if (bank->drv[pin_num / 8].drv_type & DRV_TYPE_WRITABLE_32BIT) {
+regmap_read(regmap, reg, );
+data &= ~(((1 << rmask_bits) - 1) << bit);
+} else {
+/* enable the write to the equivalent lower bits */
+data = ((1 << rmask_bits) - 1) << (bit + 16);
+}

+data |= (ret << bit);
ret = regmap_write(regmap, reg, data);
return ret;
}
@@ -375,7 +387,11 @@ static int rockchip_set_pull(struct 
rockchip_pin_bank *bank,

case RK3288:
case RK3368:
case RK3399:
-pull_type = bank->pull_type[pin_num / 8];
+/*
+* Where need to clean the special mask for
+* rockchip_pull_list.
+*/
+pull_type = bank->pull_type[pin_num / 8] & (~PULL_TYPE_IO_MASK);
ret = -EINVAL;
for (i = 0; i < ARRAY_SIZE(rockchip_pull_list[pull_type]);
i++) {
@@ -390,10 +406,15 @@ static int rockchip_set_pull(struct 
rockchip_pin_bank *bank,

return ret;
}

-/* enable the write to the equivalent lower bits */
-data = ((1 << ROCKCHIP_PULL_BITS_PER_PIN) - 1) << (bit + 16);
-data |= (ret << bit);
+if (bank->pull_type[pin_num / 8] & PULL_TYPE_WRITABLE_32BIT) {
+regmap_read(regmap, reg, );
+data &= ~(((1 << ROCKCHIP_PULL_BITS_PER_PIN) - 1) << bit);
+} else {
+/* enable the write to the equivalent lower bits */
+data = ((1 << ROCKCHIP_PULL_BITS_PER_PIN) - 1) << (bit + 16);
+}

+data |= (ret << bit);
ret = regmap_write(regmap, reg, data);
break;
default:
diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip.h 
b/drivers/pinctrl/rockchip/pinctrl-rockchip.h

index bc809630c1..5a6849c996 100644
--- a/drivers/pinctrl/rockchip/pinctrl-rockchip.h

Re: [U-Boot] [PATCH] pinctrl: rockchip: Add 32bit writing function for rk3288 gpio0 pinctrl

[Philipp Tomsich]

David,

I am applying this one as a last-minute fix for 2019.4.
However, I’ll still need the v2 for the next cycle, as I my review comments 
still apply.

Thanks,
Philipp.

> On 12.02.2019, at 12:55, Philipp Tomsich 
>  wrote:
> 
> 
> 
>> On 12.02.2019, at 12:51, David Wu  wrote:
>> 
>> There are no higher 16 writing corresponding bits for pmu_gpio0's
>> iomux/drive/pull at rk3288, need to read the value from register
>> firstly. Add the flag to distinguish it from normal registers.
>> 
>> Signed-off-by: David Wu 
>> ---
>> drivers/pinctrl/rockchip/pinctrl-rk3288.c | 17 ++--
>> .../pinctrl/rockchip/pinctrl-rockchip-core.c  | 39 ++-
>> drivers/pinctrl/rockchip/pinctrl-rockchip.h   | 33 
>> 3 files changed, 76 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c 
>> b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
>> index 60585f3208..8b6ce11a63 100644
>> --- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c
>> +++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
>> @@ -92,10 +92,19 @@ static void rk3288_calc_drv_reg_and_bit(struct 
>> rockchip_pin_bank *bank,
>> }
>> 
>> static struct rockchip_pin_bank rk3288_pin_banks[] = {
>> -PIN_BANK_IOMUX_FLAGS(0, 24, "gpio0", IOMUX_SOURCE_PMU,
>> - IOMUX_SOURCE_PMU,
>> - IOMUX_SOURCE_PMU,
>> - IOMUX_UNROUTED
>> +PIN_BANK_IOMUX_DRV_PULL_FLAGS(0, 24, "gpio0",
>> +  IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
>> +  IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
>> +  IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
>> +  IOMUX_UNROUTED,
>> +  DRV_TYPE_WRITABLE_32BIT,
>> +  DRV_TYPE_WRITABLE_32BIT,
>> +  DRV_TYPE_WRITABLE_32BIT,
>> +  0,
>> +  PULL_TYPE_WRITABLE_32BIT,
>> +  PULL_TYPE_WRITABLE_32BIT,
>> +  PULL_TYPE_WRITABLE_32BIT,
>> +  0
>>  ),
>>  PIN_BANK_IOMUX_FLAGS(1, 32, "gpio1", IOMUX_UNROUTED,
>>   IOMUX_UNROUTED,
>> diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c 
>> b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
>> index b84b079064..ce935656f0 100644
>> --- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
>> +++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
>> @@ -228,7 +228,13 @@ static int rockchip_set_mux(struct rockchip_pin_bank 
>> *bank, int pin, int mux)
>>  }
>>  }
>> 
>> -data = (mask << (bit + 16));
>> +if (mux_type & IOMUX_WRITABLE_32BIT) {
>> +regmap_read(regmap, reg, );
>> +data &= ~(mask << bit);
>> +} else {
>> +data = (mask << (bit + 16));
>> +}
>> +
> 
> This can not be optimised out by the compiler (for all the other platforms 
> that don’t need it).
> Please structure this (and the entire driver) to not pull in unneeded baggage 
> that is only used
> by one or a few devices.
> 
>>  data |= (mux & mask) << bit;
>>  ret = regmap_write(regmap, reg, data);
>> 
>> @@ -252,7 +258,8 @@ static int rockchip_set_drive_perpin(struct 
>> rockchip_pin_bank *bank,
>>  int reg, ret, i;
>>  u32 data, rmask_bits, temp;
>>  u8 bit;
>> -int drv_type = bank->drv[pin_num / 8].drv_type;
>> +/* Where need to clean the special mask for rockchip_perpin_drv_list */
>> +int drv_type = bank->drv[pin_num / 8].drv_type & (~DRV_TYPE_IO_MASK);
>> 
>>  debug("setting drive of GPIO%d-%d to %d\n", bank->bank_num,
>>pin_num, strength);
>> @@ -324,10 +331,15 @@ static int rockchip_set_drive_perpin(struct 
>> rockchip_pin_bank *bank,
>>  return -EINVAL;
>>  }
>> 
>> -/* enable the write to the equivalent lower bits */
>> -data = ((1 << rmask_bits) - 1) << (bit + 16);
>> -data |= (ret << bit);
>> +if (bank->drv[pin_num / 8].drv_type & DRV_TYPE_WRITABLE_32BIT) {
>> +regmap_read(regmap, reg, );
>> +data &= ~(((1 << rmask_bits) - 1) << bit);
>> +} else {
>> +/* enable the write to the equivalent lower bits */
>> +data = ((1 << rmask_bits) - 1) << (bit + 16);
>> +}
>> 
>> +data |= (ret << bit);
>>  ret = regmap_write(regmap, reg, data);
>>  return ret;
>> }
>> @@ -375,7 +387,11 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
>> *bank,
>>  case RK3288:
>>  case RK3368:
>>  case RK3399:
>> -pull_type = bank->pull_type[pin_num / 8];
>> +/*
>> + * Where need to clean the special mask for
>> + * rockchip_pull_list.
>> + */
>> +pull_type = 

Re: [U-Boot] [PATCH] pinctrl: rockchip: Add 32bit writing function for rk3288 gpio0 pinctrl

[Philipp Tomsich]

> On 12.02.2019, at 12:51, David Wu  wrote:
> 
> There are no higher 16 writing corresponding bits for pmu_gpio0's
> iomux/drive/pull at rk3288, need to read the value from register
> firstly. Add the flag to distinguish it from normal registers.
> 
> Signed-off-by: David Wu 
> ---
> drivers/pinctrl/rockchip/pinctrl-rk3288.c | 17 ++--
> .../pinctrl/rockchip/pinctrl-rockchip-core.c  | 39 ++-
> drivers/pinctrl/rockchip/pinctrl-rockchip.h   | 33 
> 3 files changed, 76 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c 
> b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
> index 60585f3208..8b6ce11a63 100644
> --- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c
> +++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
> @@ -92,10 +92,19 @@ static void rk3288_calc_drv_reg_and_bit(struct 
> rockchip_pin_bank *bank,
> }
> 
> static struct rockchip_pin_bank rk3288_pin_banks[] = {
> - PIN_BANK_IOMUX_FLAGS(0, 24, "gpio0", IOMUX_SOURCE_PMU,
> -  IOMUX_SOURCE_PMU,
> -  IOMUX_SOURCE_PMU,
> -  IOMUX_UNROUTED
> + PIN_BANK_IOMUX_DRV_PULL_FLAGS(0, 24, "gpio0",
> +   IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
> +   IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
> +   IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
> +   IOMUX_UNROUTED,
> +   DRV_TYPE_WRITABLE_32BIT,
> +   DRV_TYPE_WRITABLE_32BIT,
> +   DRV_TYPE_WRITABLE_32BIT,
> +   0,
> +   PULL_TYPE_WRITABLE_32BIT,
> +   PULL_TYPE_WRITABLE_32BIT,
> +   PULL_TYPE_WRITABLE_32BIT,
> +   0
>   ),
>   PIN_BANK_IOMUX_FLAGS(1, 32, "gpio1", IOMUX_UNROUTED,
>IOMUX_UNROUTED,
> diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c 
> b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> index b84b079064..ce935656f0 100644
> --- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> +++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> @@ -228,7 +228,13 @@ static int rockchip_set_mux(struct rockchip_pin_bank 
> *bank, int pin, int mux)
>   }
>   }
> 
> - data = (mask << (bit + 16));
> + if (mux_type & IOMUX_WRITABLE_32BIT) {
> + regmap_read(regmap, reg, );
> + data &= ~(mask << bit);
> + } else {
> + data = (mask << (bit + 16));
> + }
> +

This can not be optimised out by the compiler (for all the other platforms that 
don’t need it).
Please structure this (and the entire driver) to not pull in unneeded baggage 
that is only used
by one or a few devices.

>   data |= (mux & mask) << bit;
>   ret = regmap_write(regmap, reg, data);
> 
> @@ -252,7 +258,8 @@ static int rockchip_set_drive_perpin(struct 
> rockchip_pin_bank *bank,
>   int reg, ret, i;
>   u32 data, rmask_bits, temp;
>   u8 bit;
> - int drv_type = bank->drv[pin_num / 8].drv_type;
> + /* Where need to clean the special mask for rockchip_perpin_drv_list */
> + int drv_type = bank->drv[pin_num / 8].drv_type & (~DRV_TYPE_IO_MASK);
> 
>   debug("setting drive of GPIO%d-%d to %d\n", bank->bank_num,
> pin_num, strength);
> @@ -324,10 +331,15 @@ static int rockchip_set_drive_perpin(struct 
> rockchip_pin_bank *bank,
>   return -EINVAL;
>   }
> 
> - /* enable the write to the equivalent lower bits */
> - data = ((1 << rmask_bits) - 1) << (bit + 16);
> - data |= (ret << bit);
> + if (bank->drv[pin_num / 8].drv_type & DRV_TYPE_WRITABLE_32BIT) {
> + regmap_read(regmap, reg, );
> + data &= ~(((1 << rmask_bits) - 1) << bit);
> + } else {
> + /* enable the write to the equivalent lower bits */
> + data = ((1 << rmask_bits) - 1) << (bit + 16);
> + }
> 
> + data |= (ret << bit);
>   ret = regmap_write(regmap, reg, data);
>   return ret;
> }
> @@ -375,7 +387,11 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
> *bank,
>   case RK3288:
>   case RK3368:
>   case RK3399:
> - pull_type = bank->pull_type[pin_num / 8];
> + /*
> +  * Where need to clean the special mask for
> +  * rockchip_pull_list.
> +  */
> + pull_type = bank->pull_type[pin_num / 8] & (~PULL_TYPE_IO_MASK);
>   ret = -EINVAL;
>   for (i = 0; i < ARRAY_SIZE(rockchip_pull_list[pull_type]);
>   i++) {
> @@ -390,10 +406,15 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
> *bank,
>   

[U-Boot] [PATCH] pinctrl: rockchip: Add 32bit writing function for rk3288 gpio0 pinctrl

[David Wu]

There are no higher 16 writing corresponding bits for pmu_gpio0's
iomux/drive/pull at rk3288, need to read the value from register
firstly. Add the flag to distinguish it from normal registers.

Signed-off-by: David Wu 
---
 drivers/pinctrl/rockchip/pinctrl-rk3288.c | 17 ++--
 .../pinctrl/rockchip/pinctrl-rockchip-core.c  | 39 ++-
 drivers/pinctrl/rockchip/pinctrl-rockchip.h   | 33 
 3 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/rockchip/pinctrl-rk3288.c 
b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
index 60585f3208..8b6ce11a63 100644
--- a/drivers/pinctrl/rockchip/pinctrl-rk3288.c
+++ b/drivers/pinctrl/rockchip/pinctrl-rk3288.c
@@ -92,10 +92,19 @@ static void rk3288_calc_drv_reg_and_bit(struct 
rockchip_pin_bank *bank,
 }
 
 static struct rockchip_pin_bank rk3288_pin_banks[] = {
-   PIN_BANK_IOMUX_FLAGS(0, 24, "gpio0", IOMUX_SOURCE_PMU,
-IOMUX_SOURCE_PMU,
-IOMUX_SOURCE_PMU,
-IOMUX_UNROUTED
+   PIN_BANK_IOMUX_DRV_PULL_FLAGS(0, 24, "gpio0",
+ IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
+ IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
+ IOMUX_SOURCE_PMU | IOMUX_WRITABLE_32BIT,
+ IOMUX_UNROUTED,
+ DRV_TYPE_WRITABLE_32BIT,
+ DRV_TYPE_WRITABLE_32BIT,
+ DRV_TYPE_WRITABLE_32BIT,
+ 0,
+ PULL_TYPE_WRITABLE_32BIT,
+ PULL_TYPE_WRITABLE_32BIT,
+ PULL_TYPE_WRITABLE_32BIT,
+ 0
),
PIN_BANK_IOMUX_FLAGS(1, 32, "gpio1", IOMUX_UNROUTED,
 IOMUX_UNROUTED,
diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c 
b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
index b84b079064..ce935656f0 100644
--- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
+++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
@@ -228,7 +228,13 @@ static int rockchip_set_mux(struct rockchip_pin_bank 
*bank, int pin, int mux)
}
}
 
-   data = (mask << (bit + 16));
+   if (mux_type & IOMUX_WRITABLE_32BIT) {
+   regmap_read(regmap, reg, );
+   data &= ~(mask << bit);
+   } else {
+   data = (mask << (bit + 16));
+   }
+
data |= (mux & mask) << bit;
ret = regmap_write(regmap, reg, data);
 
@@ -252,7 +258,8 @@ static int rockchip_set_drive_perpin(struct 
rockchip_pin_bank *bank,
int reg, ret, i;
u32 data, rmask_bits, temp;
u8 bit;
-   int drv_type = bank->drv[pin_num / 8].drv_type;
+   /* Where need to clean the special mask for rockchip_perpin_drv_list */
+   int drv_type = bank->drv[pin_num / 8].drv_type & (~DRV_TYPE_IO_MASK);
 
debug("setting drive of GPIO%d-%d to %d\n", bank->bank_num,
  pin_num, strength);
@@ -324,10 +331,15 @@ static int rockchip_set_drive_perpin(struct 
rockchip_pin_bank *bank,
return -EINVAL;
}
 
-   /* enable the write to the equivalent lower bits */
-   data = ((1 << rmask_bits) - 1) << (bit + 16);
-   data |= (ret << bit);
+   if (bank->drv[pin_num / 8].drv_type & DRV_TYPE_WRITABLE_32BIT) {
+   regmap_read(regmap, reg, );
+   data &= ~(((1 << rmask_bits) - 1) << bit);
+   } else {
+   /* enable the write to the equivalent lower bits */
+   data = ((1 << rmask_bits) - 1) << (bit + 16);
+   }
 
+   data |= (ret << bit);
ret = regmap_write(regmap, reg, data);
return ret;
 }
@@ -375,7 +387,11 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
*bank,
case RK3288:
case RK3368:
case RK3399:
-   pull_type = bank->pull_type[pin_num / 8];
+   /*
+* Where need to clean the special mask for
+* rockchip_pull_list.
+*/
+   pull_type = bank->pull_type[pin_num / 8] & (~PULL_TYPE_IO_MASK);
ret = -EINVAL;
for (i = 0; i < ARRAY_SIZE(rockchip_pull_list[pull_type]);
i++) {
@@ -390,10 +406,15 @@ static int rockchip_set_pull(struct rockchip_pin_bank 
*bank,
return ret;
}
 
-   /* enable the write to the equivalent lower bits */
-   data = ((1 << ROCKCHIP_PULL_BITS_PER_PIN) - 1) << (bit + 16);
-   data |= (ret << bit);
+   if (bank->pull_type[pin_num / 8] & PULL_TYPE_WRITABLE_32BIT) {
+   regmap_read(regmap,