Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320865 > unrolled thread

Re: [PATCH v2] pinctrl: rockchip: add support for the rk3399

Started byHeiko Stübner <heiko@sntech.de>
First post2016-01-28 18:00 +0100
Last post2016-01-29 12:40 +0100
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] pinctrl: rockchip: add support for the rk3399 Heiko Stübner <heiko@sntech.de> - 2016-01-28 18:00 +0100
    Re: [PATCH v2] pinctrl: rockchip: add support for the rk3399 "David.Wu" <wdc@rock-chips.com> - 2016-01-29 12:40 +0100

#1320865 — Re: [PATCH v2] pinctrl: rockchip: add support for the rk3399

FromHeiko Stübner <heiko@sntech.de>
Date2016-01-28 18:00 +0100
SubjectRe: [PATCH v2] pinctrl: rockchip: add support for the rk3399
Message-ID<qVY5k-6dI-21@gated-at.bofh.it>
Hi David,

Am Donnerstag, 7. Januar 2016, 15:41:38 schrieb David Wu:
> From: David Wu <david.wu@rock-chips.com>
> 
> The pinctrl of rk3399 is much different from other's,
> especially the 3bits of drive strength.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> change from v1:
> - need spin_unlock_irqrestore for set drive default case
> 
>  .../bindings/pinctrl/rockchip,pinctrl.txt          |   1 +
>  drivers/pinctrl/pinctrl-rockchip.c                 | 339
> ++++++++++++++++++++- 2 files changed, 326 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
> b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt index
> 391ef4b..3bb9456 100644
> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
> @@ -22,6 +22,7 @@ Required properties for iomux controller:
>    - compatible: one of "rockchip,rk2928-pinctrl",
> "rockchip,rk3066a-pinctrl" "rockchip,rk3066b-pinctrl",
> "rockchip,rk3188-pinctrl"
>  		       "rockchip,rk3288-pinctrl", "rockchip,rk3368-pinctrl"
> +		       "rockchip,rk3399-pinctrl"
>    - rockchip,grf: phandle referencing a syscon providing the
>  	 "general register files"
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
> b/drivers/pinctrl/pinctrl-rockchip.c index a065112..9d61c6e 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c

[...]

> @@ -685,19 +821,60 @@ static int rockchip_get_drive_perpin(struct
> rockchip_pin_bank *bank, struct rockchip_pin_ctrl *ctrl = info->ctrl;
>  	struct regmap *regmap;
>  	int reg, ret;
> -	u32 data;
> +	u32 data, temp, rmask_bits;
>  	u8 bit;
> +	int drv_type = bank->drv[pin_num / 8].drv_type;
> 
>  	ctrl->drv_calc_reg(bank, pin_num, &regmap, &reg, &bit);
> 
> +	switch (drv_type) {
> +	case DRV_TYPE_IO_1V8_3V0_AUTO:
> +	case DRV_TYPE_IO_3V3_ONLY:
> +		if (RK3399_DRV_3BITS_PER_PIN + bit >= 16) {

According to the doc excerpts, wouldn't the big special handling only be 
necessary for for gpio3b5 alone (+ other banks) ... i.e. the one setting being 
split over both registers?
If so I think doing something like the following might be easier to 
understand? Same of course for the set-counterpart. [Beware if I missed up the 
indices somewhere :-) ]

rmask_bits = RK3399_DRV_3BITS_PER_PIN;
switch(bit) {
case 0 ... 12:
	/* regular case, nothing to do */
	break;
case 15:
	/* drive-strength offset is special, as it is spread over 2 registers */
	ret = regmap_read(regmap, reg, &data);
	if (ret)
		return ret;

	ret = regmap_read(regmap, reg + 0x4, &temp);
	if (ret)
		return ret;

	/*
	 * the bit data[15] contains bit 0 of the value
	 * while temp[1:0] containts bits 2 and 1
	 */
	data >>= 15;
	temp &= 0x3;
	temp <<= 1;
	data |= temp;

	return rockchip_perpin_drv_list[drv_type][data];
case 18 ... 21:
	/* setting fully enclosed in the second register */
	rmask_bits = RK3399_DRV_3BITS_PER_PIN;
	reg += 4;
	bit -= 18;
}

> +			/* need to read regs twice */
> +			ret = regmap_read(regmap, reg, &temp);
> +			if (ret)
> +				return ret;
> +
> +			temp >>= bit;
> +			rmask_bits = 16 - bit;
> +			temp &= (1 << rmask_bits) - 1;
> +
> +			reg = reg + 0x4;
> +			ret = regmap_read(regmap, reg, &data);
> +			if (ret)
> +				return ret;
> +
> +			rmask_bits = RK3399_DRV_3BITS_PER_PIN
> +				+ bit - 16;
> +			data &= (1 << rmask_bits) - 1;
> +			data <<= 16 - bit;
> +			data |= temp;
> +
> +			return rockchip_perpin_drv_list[drv_type][data];
> +		}
> +
> +		rmask_bits = RK3399_DRV_3BITS_PER_PIN;
> +		break;
> +	case DRV_TYPE_IO_DEFAULT:
> +	case DRV_TYPE_IO_1V8_OR_3V0:
> +	case DRV_TYPE_IO_1V8_ONLY:
> +		rmask_bits = RK3288_DRV_BITS_PER_PIN;
> +		break;
> +	default:
> +		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
> +			drv_type);
> +		return -EINVAL;
> +	}
> +
>  	ret = regmap_read(regmap, reg, &data);
>  	if (ret)
>  		return ret;
> 
>  	data >>= bit;
> -	data &= (1 << RK3288_DRV_BITS_PER_PIN) - 1;
> +	data &= (1 << rmask_bits) - 1;
> 
> -	return rockchip_perpin_drv_list[data];
> +	return rockchip_perpin_drv_list[drv_type][data];
>  }
> 
>  static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,

[...]

> @@ -729,8 +913,45 @@ static int rockchip_set_drive_perpin(struct
> rockchip_pin_bank *bank,
> 
>  	spin_lock_irqsave(&bank->slock, flags);
> 
> +	switch (drv_type) {
> +	case DRV_TYPE_IO_1V8_3V0_AUTO:
> +	case DRV_TYPE_IO_3V3_ONLY:
> +		if (RK3399_DRV_3BITS_PER_PIN + bit >= 16) {
> +			/* need to write regs twice */
> +			rmask_bits = 16 - bit;
> +			/* enable the write to the equivalent lower bits */
> +			data = ((1 << rmask_bits) - 1) << (bit + 16);
> +			rmask = data | (data >> 16);
> +			ret &= (1 << rmask_bits) - 1;
> +			data |= (ret << bit);
> +
> +			ret = regmap_update_bits(regmap, reg, rmask, data);
> +			if (ret)
> +				return ret;
> +
> +			ret = i >> rmask_bits;
> +			rmask_bits = RK3399_DRV_3BITS_PER_PIN + bit - 16;
> +			reg = reg + 0x4;
> +			bit = 0;

Apart from a change similar to the above, should the setting maybe also 
directly return and not depend on the final write?

> +		} else {
> +			rmask_bits = RK3399_DRV_3BITS_PER_PIN;
> +		}
> +		break;
> +	case DRV_TYPE_IO_DEFAULT:
> +	case DRV_TYPE_IO_1V8_OR_3V0:
> +	case DRV_TYPE_IO_1V8_ONLY:
> +		rmask_bits = RK3288_DRV_BITS_PER_PIN;
> +		break;
> +	default:
> +		spin_unlock_irqrestore(&bank->slock, flags);
> +		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
> +			drv_type);
> +		return -EINVAL;
> +
> +	}
> +
>  	/* enable the write to the equivalent lower bits */
> -	data = ((1 << RK3288_DRV_BITS_PER_PIN) - 1) << (bit + 16);
> +	data = ((1 << rmask_bits) - 1) << (bit + 16);
>  	rmask = data | (data >> 16);
>  	data |= (ret << bit);
> 

[...]

> @@ -2208,6 +2461,62 @@ static struct rockchip_pin_ctrl rk3368_pin_ctrl = {
>  		.drv_calc_reg		= rk3368_calc_drv_reg_and_bit,
>  };
> 
> +static struct rockchip_pin_bank rk3399_pin_banks[] = {
> +	PIN_BANK_IOMUX_DRV_FLAGS_OFFSET(0, 32, "gpio0", IOMUX_SOURCE_PMU,
> +					IOMUX_SOURCE_PMU,
> +					IOMUX_SOURCE_PMU,
> +					IOMUX_SOURCE_PMU,
> +					DRV_TYPE_IO_1V8_ONLY,
> +					DRV_TYPE_IO_1V8_ONLY,
> +					0,
> +					0,

DRV_TYPE_IO_DEFAULT instead of 0 please.

[toc] | [next] | [standalone]

#1321681

From"David.Wu" <wdc@rock-chips.com>
Date2016-01-29 12:40 +0100
Message-ID<qWfzd-2gW-25@gated-at.bofh.it>
In reply to#1320865
Hi Heiko,

在 2016/1/29 0:56, Heiko Stübner 写道:
> Hi David,
>
> Am Donnerstag, 7. Januar 2016, 15:41:38 schrieb David Wu:
>> From: David Wu <david.wu@rock-chips.com>
>>
>> The pinctrl of rk3399 is much different from other's,
>> especially the 3bits of drive strength.
>>
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>> ---
>> change from v1:
>> - need spin_unlock_irqrestore for set drive default case
>>
>>   .../bindings/pinctrl/rockchip,pinctrl.txt          |   1 +
>>   drivers/pinctrl/pinctrl-rockchip.c                 | 339
>> ++++++++++++++++++++- 2 files changed, 326 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
>> b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt index
>> 391ef4b..3bb9456 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.txt
>> @@ -22,6 +22,7 @@ Required properties for iomux controller:
>>     - compatible: one of "rockchip,rk2928-pinctrl",
>> "rockchip,rk3066a-pinctrl" "rockchip,rk3066b-pinctrl",
>> "rockchip,rk3188-pinctrl"
>>   		       "rockchip,rk3288-pinctrl", "rockchip,rk3368-pinctrl"
>> +		       "rockchip,rk3399-pinctrl"
>>     - rockchip,grf: phandle referencing a syscon providing the
>>   	 "general register files"
>>
>> diff --git a/drivers/pinctrl/pinctrl-rockchip.c
>> b/drivers/pinctrl/pinctrl-rockchip.c index a065112..9d61c6e 100644
>> --- a/drivers/pinctrl/pinctrl-rockchip.c
>> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> [...]
>
>> @@ -685,19 +821,60 @@ static int rockchip_get_drive_perpin(struct
>> rockchip_pin_bank *bank, struct rockchip_pin_ctrl *ctrl = info->ctrl;
>>   	struct regmap *regmap;
>>   	int reg, ret;
>> -	u32 data;
>> +	u32 data, temp, rmask_bits;
>>   	u8 bit;
>> +	int drv_type = bank->drv[pin_num / 8].drv_type;
>>
>>   	ctrl->drv_calc_reg(bank, pin_num, &regmap, &reg, &bit);
>>
>> +	switch (drv_type) {
>> +	case DRV_TYPE_IO_1V8_3V0_AUTO:
>> +	case DRV_TYPE_IO_3V3_ONLY:
>> +		if (RK3399_DRV_3BITS_PER_PIN + bit >= 16) {
> According to the doc excerpts, wouldn't the big special handling only be
> necessary for for gpio3b5 alone (+ other banks) ... i.e. the one setting being
> split over both registers?

yes, only the pin5 setting of 3bits width bank is split over two registers.

> If so I think doing something like the following might be easier to
> understand? Same of course for the set-counterpart. [Beware if I missed up the
> indices somewhere :-)

yeap.
Seemed it is clearer to view by using following code.
I will have a try, thanks.

>
> rmask_bits = RK3399_DRV_3BITS_PER_PIN;
> switch(bit) {
> case 0 ... 12:
> 	/* regular case, nothing to do */
> 	break;
> case 15:
> 	/* drive-strength offset is special, as it is spread over 2 registers */
> 	ret = regmap_read(regmap, reg, &data);
> 	if (ret)
> 		return ret;
>
> 	ret = regmap_read(regmap, reg + 0x4, &temp);
> 	if (ret)
> 		return ret;
>
> 	/*
> 	 * the bit data[15] contains bit 0 of the value
> 	 * while temp[1:0] containts bits 2 and 1
> 	 */
> 	data >>= 15;
> 	temp &= 0x3;
> 	temp <<= 1;
> 	data |= temp;
>
> 	return rockchip_perpin_drv_list[drv_type][data];
> case 18 ... 21:
> 	/* setting fully enclosed in the second register */
> 	rmask_bits = RK3399_DRV_3BITS_PER_PIN;
> 	reg += 4;
> 	bit -= 18;
> }
>
>> +			/* need to read regs twice */
>> +			ret = regmap_read(regmap, reg, &temp);
>> +			if (ret)
>> +				return ret;
>> +
>> +			temp >>= bit;
>> +			rmask_bits = 16 - bit;
>> +			temp &= (1 << rmask_bits) - 1;
>> +
>> +			reg = reg + 0x4;
>> +			ret = regmap_read(regmap, reg, &data);
>> +			if (ret)
>> +				return ret;
>> +
>> +			rmask_bits = RK3399_DRV_3BITS_PER_PIN
>> +				+ bit - 16;
>> +			data &= (1 << rmask_bits) - 1;
>> +			data <<= 16 - bit;
>> +			data |= temp;
>> +
>> +			return rockchip_perpin_drv_list[drv_type][data];
>> +		}
>> +
>> +		rmask_bits = RK3399_DRV_3BITS_PER_PIN;
>> +		break;
>> +	case DRV_TYPE_IO_DEFAULT:
>> +	case DRV_TYPE_IO_1V8_OR_3V0:
>> +	case DRV_TYPE_IO_1V8_ONLY:
>> +		rmask_bits = RK3288_DRV_BITS_PER_PIN;
>> +		break;
>> +	default:
>> +		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
>> +			drv_type);
>> +		return -EINVAL;
>> +	}
>> +
>>   	ret = regmap_read(regmap, reg, &data);
>>   	if (ret)
>>   		return ret;
>>
>>   	data >>= bit;
>> -	data &= (1 << RK3288_DRV_BITS_PER_PIN) - 1;
>> +	data &= (1 << rmask_bits) - 1;
>>
>> -	return rockchip_perpin_drv_list[data];
>> +	return rockchip_perpin_drv_list[drv_type][data];
>>   }
>>
>>   static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank,
> [...]
>
>> @@ -729,8 +913,45 @@ static int rockchip_set_drive_perpin(struct
>> rockchip_pin_bank *bank,
>>
>>   	spin_lock_irqsave(&bank->slock, flags);
>>
>> +	switch (drv_type) {
>> +	case DRV_TYPE_IO_1V8_3V0_AUTO:
>> +	case DRV_TYPE_IO_3V3_ONLY:
>> +		if (RK3399_DRV_3BITS_PER_PIN + bit >= 16) {
>> +			/* need to write regs twice */
>> +			rmask_bits = 16 - bit;
>> +			/* enable the write to the equivalent lower bits */
>> +			data = ((1 << rmask_bits) - 1) << (bit + 16);
>> +			rmask = data | (data >> 16);
>> +			ret &= (1 << rmask_bits) - 1;
>> +			data |= (ret << bit);
>> +
>> +			ret = regmap_update_bits(regmap, reg, rmask, data);
>> +			if (ret)
>> +				return ret;
>> +
>> +			ret = i >> rmask_bits;
>> +			rmask_bits = RK3399_DRV_3BITS_PER_PIN + bit - 16;
>> +			reg = reg + 0x4;
>> +			bit = 0;
> Apart from a change similar to the above, should the setting maybe also
> directly return and not depend on the final write?

okay, i will do the simialr change to the above.

>> +		} else {
>> +			rmask_bits = RK3399_DRV_3BITS_PER_PIN;
>> +		}
>> +		break;
>> +	case DRV_TYPE_IO_DEFAULT:
>> +	case DRV_TYPE_IO_1V8_OR_3V0:
>> +	case DRV_TYPE_IO_1V8_ONLY:
>> +		rmask_bits = RK3288_DRV_BITS_PER_PIN;
>> +		break;
>> +	default:
>> +		spin_unlock_irqrestore(&bank->slock, flags);
>> +		dev_err(info->dev, "unsupported pinctrl drive type: %d\n",
>> +			drv_type);
>> +		return -EINVAL;
>> +
>> +	}
>> +
>>   	/* enable the write to the equivalent lower bits */
>> -	data = ((1 << RK3288_DRV_BITS_PER_PIN) - 1) << (bit + 16);
>> +	data = ((1 << rmask_bits) - 1) << (bit + 16);
>>   	rmask = data | (data >> 16);
>>   	data |= (ret << bit);
>>
> [...]
>
>> @@ -2208,6 +2461,62 @@ static struct rockchip_pin_ctrl rk3368_pin_ctrl = {
>>   		.drv_calc_reg		= rk3368_calc_drv_reg_and_bit,
>>   };
>>
>> +static struct rockchip_pin_bank rk3399_pin_banks[] = {
>> +	PIN_BANK_IOMUX_DRV_FLAGS_OFFSET(0, 32, "gpio0", IOMUX_SOURCE_PMU,
>> +					IOMUX_SOURCE_PMU,
>> +					IOMUX_SOURCE_PMU,
>> +					IOMUX_SOURCE_PMU,
>> +					DRV_TYPE_IO_1V8_ONLY,
>> +					DRV_TYPE_IO_1V8_ONLY,
>> +					0,
>> +					0,
> DRV_TYPE_IO_DEFAULT instead of 0 please.
>
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web