The purpose of this patchset is to add drive-strength support in meson pinconf
driver. This is a new feature that was added on the g12a. It is critical for us
to support this since many functions are failing with default pad drive-strength.
The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property
'drive-strength' is expressed in mA.
So this patch add another generic property "drive-strength-uA". The change to do so
would be minimal and could be benefit to other platforms later on.
Cheers
Guillaume
Changes since v2:
- update driver-strength-uA property to be compliant with DT documentation
- rework patch series for better understanding
- rework set_bias function
Changes since v1:
- fix missing break
- implement new pinctrl generic property "drive-strength-uA"
[1] https://lkml.kernel.org/r/[email protected]
Guillaume La Roque (6):
dt-bindings: pinctrl: add a 'drive-strength-microamp' property
pinctrl: generic: add new 'drive-strength-microamp' property support
dt-bindings: pinctrl: meson: Add drive-strength-microamp property
pinctrl: meson: Rework enable/disable bias part
pinctrl: meson: add support of drive-strength-microamp
pinctrl: meson: g12a: add DS bank value
.../bindings/pinctrl/meson,pinctrl.txt | 4 +
.../bindings/pinctrl/pinctrl-bindings.txt | 3 +
drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++--
drivers/pinctrl/meson/pinctrl-meson.c | 177 +++++++++++++++---
drivers/pinctrl/meson/pinctrl-meson.h | 18 +-
drivers/pinctrl/pinconf-generic.c | 2 +
include/linux/pinctrl/pinconf-generic.h | 3 +
7 files changed, 195 insertions(+), 48 deletions(-)
--
2.17.1
Add drive-strength-microamp property support to allow drive strength in uA
Signed-off-by: Guillaume La Roque <[email protected]>
---
drivers/pinctrl/pinconf-generic.c | 2 ++
include/linux/pinctrl/pinconf-generic.h | 3 +++
2 files changed, 5 insertions(+)
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index b4f7f8a458ea..d0cbdb1ad76a 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -39,6 +39,7 @@ static const struct pin_config_item conf_items[] = {
PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false),
PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false),
PCONFDUMP(PIN_CONFIG_DRIVE_STRENGTH, "output drive strength", "mA", true),
+ PCONFDUMP(PIN_CONFIG_DRIVE_STRENGTH_UA, "output drive strength", "uA", true),
PCONFDUMP(PIN_CONFIG_INPUT_DEBOUNCE, "input debounce", "usec", true),
PCONFDUMP(PIN_CONFIG_INPUT_ENABLE, "input enabled", NULL, false),
PCONFDUMP(PIN_CONFIG_INPUT_SCHMITT, "input schmitt trigger", NULL, false),
@@ -167,6 +168,7 @@ static const struct pinconf_generic_params dt_params[] = {
{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
{ "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
+ { "drive-strength-microamp", PIN_CONFIG_DRIVE_STRENGTH_UA, 0 },
{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
{ "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
{ "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 },
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 6c0680641108..72d06d6a3099 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -55,6 +55,8 @@
* push-pull mode, the argument is ignored.
* @PIN_CONFIG_DRIVE_STRENGTH: the pin will sink or source at most the current
* passed as argument. The argument is in mA.
+ * @PIN_CONFIG_DRIVE_STRENGTH_UA: the pin will sink or source at most the current
+ * passed as argument. The argument is in uA.
* @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
* which means it will wait for signals to settle when reading inputs. The
* argument gives the debounce time in usecs. Setting the
@@ -112,6 +114,7 @@ enum pin_config_param {
PIN_CONFIG_DRIVE_OPEN_SOURCE,
PIN_CONFIG_DRIVE_PUSH_PULL,
PIN_CONFIG_DRIVE_STRENGTH,
+ PIN_CONFIG_DRIVE_STRENGTH_UA,
PIN_CONFIG_INPUT_DEBOUNCE,
PIN_CONFIG_INPUT_ENABLE,
PIN_CONFIG_INPUT_SCHMITT,
--
2.17.1
rework bias enable/disable part to prepare drive-strength integration
Signed-off-by: Guillaume La Roque <[email protected]>
---
drivers/pinctrl/meson/pinctrl-meson.c | 79 ++++++++++++++++-----------
1 file changed, 48 insertions(+), 31 deletions(-)
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 96a4a72708e4..a216a7537564 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -174,13 +174,57 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector,
return 0;
}
+static int meson_pinconf_disable_bias(struct meson_pinctrl *pc,
+ unsigned int pin)
+{
+ struct meson_bank *bank;
+ unsigned int reg, bit = 0;
+ int ret;
+
+ ret = meson_get_bank(pc, pin, &bank);
+ if (ret)
+ return ret;
+ meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®, &bit);
+ ret = regmap_update_bits(pc->reg_pullen, reg, BIT(bit), 0);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int pin,
+ bool pull_up)
+{
+ struct meson_bank *bank;
+ unsigned int reg, bit, val = 0;
+ int ret;
+
+ ret = meson_get_bank(pc, pin, &bank);
+ if (ret)
+ return ret;
+
+ meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit);
+ if (pull_up)
+ val = BIT(bit);
+
+ ret = regmap_update_bits(pc->reg_pull, reg, BIT(bit), val);
+ if (ret)
+ return ret;
+
+ meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®, &bit);
+ ret = regmap_update_bits(pc->reg_pullen, reg, BIT(bit), BIT(bit));
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
unsigned long *configs, unsigned num_configs)
{
struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
struct meson_bank *bank;
enum pin_config_param param;
- unsigned int reg, bit;
int i, ret;
ret = meson_get_bank(pc, pin, &bank);
@@ -192,44 +236,17 @@ static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
switch (param) {
case PIN_CONFIG_BIAS_DISABLE:
- dev_dbg(pc->dev, "pin %u: disable bias\n", pin);
-
- meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®,
- &bit);
- ret = regmap_update_bits(pc->reg_pullen, reg,
- BIT(bit), 0);
+ ret = meson_pinconf_disable_bias(pc, pin);
if (ret)
return ret;
break;
case PIN_CONFIG_BIAS_PULL_UP:
- dev_dbg(pc->dev, "pin %u: enable pull-up\n", pin);
-
- meson_calc_reg_and_bit(bank, pin, REG_PULLEN,
- ®, &bit);
- ret = regmap_update_bits(pc->reg_pullen, reg,
- BIT(bit), BIT(bit));
- if (ret)
- return ret;
-
- meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit);
- ret = regmap_update_bits(pc->reg_pull, reg,
- BIT(bit), BIT(bit));
+ ret = meson_pinconf_enable_bias(pc, pin, 1);
if (ret)
return ret;
break;
case PIN_CONFIG_BIAS_PULL_DOWN:
- dev_dbg(pc->dev, "pin %u: enable pull-down\n", pin);
-
- meson_calc_reg_and_bit(bank, pin, REG_PULLEN,
- ®, &bit);
- ret = regmap_update_bits(pc->reg_pullen, reg,
- BIT(bit), BIT(bit));
- if (ret)
- return ret;
-
- meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit);
- ret = regmap_update_bits(pc->reg_pull, reg,
- BIT(bit), 0);
+ ret = meson_pinconf_enable_bias(pc, pin, 0);
if (ret)
return ret;
break;
--
2.17.1
Add optional drive-strength-microamp property
Signed-off-by: Guillaume La Roque <[email protected]>
---
Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
index a47dd990a8d3..a7618605bf1e 100644
--- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
@@ -51,6 +51,10 @@ Configuration nodes support the generic properties "bias-disable",
"bias-pull-up" and "bias-pull-down", described in file
pinctrl-bindings.txt
+Optional properties :
+ - drive-strength-microamp: Drive strength for the specified pins in uA.
+ This property is only valid for G12A and newer.
+
=== Example ===
pinctrl: pinctrl@c1109880 {
--
2.17.1
On Tue, 2019-05-07 at 13:57 +0200, Guillaume La Roque wrote:
> The purpose of this patchset is to add drive-strength support in meson pinconf
> driver. This is a new feature that was added on the g12a. It is critical for us
> to support this since many functions are failing with default pad drive-strength.
>
> The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property
> 'drive-strength' is expressed in mA.
> So this patch add another generic property "drive-strength-uA". The change to do so
> would be minimal and could be benefit to other platforms later on.
>
> Cheers
> Guillaume
>
> Changes since v2:
> - update driver-strength-uA property to be compliant with DT documentation
> - rework patch series for better understanding
> - rework set_bias function
>
> Changes since v1:
> - fix missing break
> - implement new pinctrl generic property "drive-strength-uA"
>
> [1] https://lkml.kernel.org/r/[email protected]
>
>
> Guillaume La Roque (6):
> dt-bindings: pinctrl: add a 'drive-strength-microamp' property
> pinctrl: generic: add new 'drive-strength-microamp' property support
> dt-bindings: pinctrl: meson: Add drive-strength-microamp property
> pinctrl: meson: Rework enable/disable bias part
> pinctrl: meson: add support of drive-strength-microamp
> pinctrl: meson: g12a: add DS bank value
>
> .../bindings/pinctrl/meson,pinctrl.txt | 4 +
> .../bindings/pinctrl/pinctrl-bindings.txt | 3 +
> drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++--
> drivers/pinctrl/meson/pinctrl-meson.c | 177 +++++++++++++++---
> drivers/pinctrl/meson/pinctrl-meson.h | 18 +-
> drivers/pinctrl/pinconf-generic.c | 2 +
> include/linux/pinctrl/pinconf-generic.h | 3 +
> 7 files changed, 195 insertions(+), 48 deletions(-)
>
Tested-by: Jerome Brunet <[email protected]>
On Tue, May 7, 2019 at 1:57 PM Guillaume La Roque <[email protected]> wrote:
>
> Add optional drive-strength-microamp property
>
> Signed-off-by: Guillaume La Roque <[email protected]>
Reviewed-by: Martin Blumenstingl <[email protected]>
Hi Guillaume,
On Tue, May 7, 2019 at 1:57 PM Guillaume La Roque <[email protected]> wrote:
>
> rework bias enable/disable part to prepare drive-strength integration
if it was my patch I would add "no functional changes" at the end to
make it explicit that this only changes the structure of the code.
>
> Signed-off-by: Guillaume La Roque <[email protected]>
with the minor comments from below addressed:
Reviewed-by: Martin Blumenstingl <[email protected]>
> ---
> drivers/pinctrl/meson/pinctrl-meson.c | 79 ++++++++++++++++-----------
> 1 file changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
> index 96a4a72708e4..a216a7537564 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
> @@ -174,13 +174,57 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector,
> return 0;
> }
>
> +static int meson_pinconf_disable_bias(struct meson_pinctrl *pc,
> + unsigned int pin)
> +{
> + struct meson_bank *bank;
> + unsigned int reg, bit = 0;
> + int ret;
> +
> + ret = meson_get_bank(pc, pin, &bank);
> + if (ret)
> + return ret;
add an empty line here to keep it consistent with the rest of the code
[...]
> static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
> unsigned long *configs, unsigned num_configs)
> {
> struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
> struct meson_bank *bank;
bank is not read anymore (it's passed to meson_get_bank to set it, but
then it's not read, which is probably why my compiler doesn't
complain)
> enum pin_config_param param;
> - unsigned int reg, bit;
> int i, ret;
>
> ret = meson_get_bank(pc, pin, &bank);
> @@ -192,44 +236,17 @@ static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
>
> switch (param) {
> case PIN_CONFIG_BIAS_DISABLE:
> - dev_dbg(pc->dev, "pin %u: disable bias\n", pin);
> -
> - meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®,
> - &bit);
> - ret = regmap_update_bits(pc->reg_pullen, reg,
> - BIT(bit), 0);
> + ret = meson_pinconf_disable_bias(pc, pin);
> if (ret)
> return ret;
> break;
> case PIN_CONFIG_BIAS_PULL_UP:
> - dev_dbg(pc->dev, "pin %u: enable pull-up\n", pin);
> -
> - meson_calc_reg_and_bit(bank, pin, REG_PULLEN,
> - ®, &bit);
> - ret = regmap_update_bits(pc->reg_pullen, reg,
> - BIT(bit), BIT(bit));
> - if (ret)
> - return ret;
> -
> - meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit);
> - ret = regmap_update_bits(pc->reg_pull, reg,
> - BIT(bit), BIT(bit));
> + ret = meson_pinconf_enable_bias(pc, pin, 1);
use "true" instead of "1"?
> if (ret)
> return ret;
> break;
> case PIN_CONFIG_BIAS_PULL_DOWN:
> - dev_dbg(pc->dev, "pin %u: enable pull-down\n", pin);
> -
> - meson_calc_reg_and_bit(bank, pin, REG_PULLEN,
> - ®, &bit);
> - ret = regmap_update_bits(pc->reg_pullen, reg,
> - BIT(bit), BIT(bit));
> - if (ret)
> - return ret;
> -
> - meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit);
> - ret = regmap_update_bits(pc->reg_pull, reg,
> - BIT(bit), 0);
> + ret = meson_pinconf_enable_bias(pc, pin, 0);
use "false" instead of "0"?
one overall comment: thank you for working on this!
in my opinion it's a good preparation step to ensure that
meson_pinconf_set is easy to understand even if we add more
functionality here
Regards
Martin
On Tue, May 7, 2019 at 1:57 PM Guillaume La Roque <[email protected]> wrote:
>
> The purpose of this patchset is to add drive-strength support in meson pinconf
> driver. This is a new feature that was added on the g12a. It is critical for us
> to support this since many functions are failing with default pad drive-strength.
>
> The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property
> 'drive-strength' is expressed in mA.
> So this patch add another generic property "drive-strength-uA". The change to do so
> would be minimal and could be benefit to other platforms later on.
>
> Cheers
> Guillaume
>
> Changes since v2:
> - update driver-strength-uA property to be compliant with DT documentation
> - rework patch series for better understanding
> - rework set_bias function
>
> Changes since v1:
> - fix missing break
> - implement new pinctrl generic property "drive-strength-uA"
>
> [1] https://lkml.kernel.org/r/[email protected]
>
>
> Guillaume La Roque (6):
> dt-bindings: pinctrl: add a 'drive-strength-microamp' property
> pinctrl: generic: add new 'drive-strength-microamp' property support
> dt-bindings: pinctrl: meson: Add drive-strength-microamp property
> pinctrl: meson: Rework enable/disable bias part
> pinctrl: meson: add support of drive-strength-microamp
> pinctrl: meson: g12a: add DS bank value
>
> .../bindings/pinctrl/meson,pinctrl.txt | 4 +
> .../bindings/pinctrl/pinctrl-bindings.txt | 3 +
> drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++--
> drivers/pinctrl/meson/pinctrl-meson.c | 177 +++++++++++++++---
> drivers/pinctrl/meson/pinctrl-meson.h | 18 +-
> drivers/pinctrl/pinconf-generic.c | 2 +
> include/linux/pinctrl/pinconf-generic.h | 3 +
> 7 files changed, 195 insertions(+), 48 deletions(-)
I gave this a go on one of my Meson8m2 boards:
[Meson8m2 doesn't support drive strength and still boots without any
crashes or obvious regressions]
Tested-by: Martin Blumenstingl <[email protected]>
hi Martin,
same as previous patch i will do a new serie with your comments.
On 5/7/19 7:53 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Tue, May 7, 2019 at 1:57 PM Guillaume La Roque <[email protected]> wrote:
>> rework bias enable/disable part to prepare drive-strength integration
> if it was my patch I would add "no functional changes" at the end to
> make it explicit that this only changes the structure of the code.
>
>> Signed-off-by: Guillaume La Roque <[email protected]>
> with the minor comments from below addressed:
> Reviewed-by: Martin Blumenstingl <[email protected]>
>
>> ---
>> drivers/pinctrl/meson/pinctrl-meson.c | 79 ++++++++++++++++-----------
>> 1 file changed, 48 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
>> index 96a4a72708e4..a216a7537564 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
>> @@ -174,13 +174,57 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector,
>> return 0;
>> }
>>
>> +static int meson_pinconf_disable_bias(struct meson_pinctrl *pc,
>> + unsigned int pin)
>> +{
>> + struct meson_bank *bank;
>> + unsigned int reg, bit = 0;
>> + int ret;
>> +
>> + ret = meson_get_bank(pc, pin, &bank);
>> + if (ret)
>> + return ret;
> add an empty line here to keep it consistent with the rest of the code
>
> [...]
>> static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
>> unsigned long *configs, unsigned num_configs)
>> {
>> struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
>> struct meson_bank *bank;
> bank is not read anymore (it's passed to meson_get_bank to set it, but
> then it's not read, which is probably why my compiler doesn't
> complain)
>
>> enum pin_config_param param;
>> - unsigned int reg, bit;
>> int i, ret;
>>
>> ret = meson_get_bank(pc, pin, &bank);
>> @@ -192,44 +236,17 @@ static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
>>
>> switch (param) {
>> case PIN_CONFIG_BIAS_DISABLE:
>> - dev_dbg(pc->dev, "pin %u: disable bias\n", pin);
>> -
>> - meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®,
>> - &bit);
>> - ret = regmap_update_bits(pc->reg_pullen, reg,
>> - BIT(bit), 0);
>> + ret = meson_pinconf_disable_bias(pc, pin);
>> if (ret)
>> return ret;
>> break;
>> case PIN_CONFIG_BIAS_PULL_UP:
>> - dev_dbg(pc->dev, "pin %u: enable pull-up\n", pin);
>> -
>> - meson_calc_reg_and_bit(bank, pin, REG_PULLEN,
>> - ®, &bit);
>> - ret = regmap_update_bits(pc->reg_pullen, reg,
>> - BIT(bit), BIT(bit));
>> - if (ret)
>> - return ret;
>> -
>> - meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit);
>> - ret = regmap_update_bits(pc->reg_pull, reg,
>> - BIT(bit), BIT(bit));
>> + ret = meson_pinconf_enable_bias(pc, pin, 1);
> use "true" instead of "1"?
>
>> if (ret)
>> return ret;
>> break;
>> case PIN_CONFIG_BIAS_PULL_DOWN:
>> - dev_dbg(pc->dev, "pin %u: enable pull-down\n", pin);
>> -
>> - meson_calc_reg_and_bit(bank, pin, REG_PULLEN,
>> - ®, &bit);
>> - ret = regmap_update_bits(pc->reg_pullen, reg,
>> - BIT(bit), BIT(bit));
>> - if (ret)
>> - return ret;
>> -
>> - meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit);
>> - ret = regmap_update_bits(pc->reg_pull, reg,
>> - BIT(bit), 0);
>> + ret = meson_pinconf_enable_bias(pc, pin, 0);
> use "false" instead of "0"?
>
> one overall comment: thank you for working on this!
> in my opinion it's a good preparation step to ensure that
> meson_pinconf_set is easy to understand even if we add more
> functionality here
>
>
> Regards
> Martin
guillaume