[v2,0/4] Add drive-strength in Meson pinctrl driver
mbox series

Message ID 20190418124758.24022-1-glaroque@baylibre.com
Headers show
Series
  • Add drive-strength in Meson pinctrl driver
Related show

Message

guillaume La Roque April 18, 2019, 12:47 p.m. UTC
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 v1:
- fix missing break
- implement new pinctrl generic property "drive-strength-uA"

[1] https://lkml.kernel.org/r/20190314163725.7918-1-jbrunet@baylibre.com

Guillaume La Roque (4):
  dt-bindings: pinctrl: add a 'drive-strength-uA' property
  pinctrl: generic: add new 'drive-strength-uA' property support
  dt-bindings: pinctrl: meson: Add drive-strength-uA property
  pinctrl: meson: add support of drive-strength-uA

 .../bindings/pinctrl/meson,pinctrl.txt        |   3 +
 .../bindings/pinctrl/pinctrl-bindings.txt     |   3 +
 drivers/pinctrl/meson/pinctrl-meson-g12a.c    |  36 ++--
 drivers/pinctrl/meson/pinctrl-meson.c         | 166 ++++++++++++++----
 drivers/pinctrl/meson/pinctrl-meson.h         |  20 ++-
 drivers/pinctrl/pinconf-generic.c             |   2 +
 include/linux/pinctrl/pinconf-generic.h       |   3 +
 7 files changed, 174 insertions(+), 59 deletions(-)

Comments

Linus Walleij April 23, 2019, 11:13 a.m. UTC | #1
On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
<glaroque@baylibre.com> 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

This needs to be reviewed by Martin Blumenstigl, Martin can you have a look
at this patch set? If you don't have the patches I think Guillaume can
resend them.

Yours,
Linus Walleij
Martin Blumenstingl April 27, 2019, 7:44 p.m. UTC | #2
Hi Guillaume,

On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> drive-strength-uA is a new feature needed for G12A SoC.
> the default DS setting after boot is usually 500uA and it is not enough for
> many functions. We need to be able to set the drive strength to reliably
> enable things like MMC, I2C, etc ...
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
I gave this a go on Meson8m2 (meaning I applied all four patches from
this series and booted the result on my board):
[Meson8m2 doesn't support drive strength and still boots without any
crashes or obvious regressions]
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

> ---
>  drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
>  drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
>  drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
personally I would have split this into two separate patches:
- one for the generic pinctrl-meson part which adds drive-strength-uA support
- another patch for enabling this on G12A

if nobody else wants you to split this then it's fine for me as well

>  3 files changed, 163 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/pinctrl/meson/pinctrl-meson-g12a.c b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
> index d494492e98e9..3475cd7bd2af 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson-g12a.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
> @@ -1304,28 +1304,28 @@ static struct meson_pmx_func meson_g12a_aobus_functions[] = {
>  };
>
>  static struct meson_bank meson_g12a_periphs_banks[] = {
> -       /* name  first  last  irq  pullen  pull  dir  out  in */
> -       BANK("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
> -            4,  0,  4,  0,  12,  0,  13, 0,  14, 0),
> -       BANK("H",    GPIOH_0,    GPIOH_8, 28, 36,
> -            3,  0,  3,  0,  9,  0,  10,  0,  11,  0),
> -       BANK("BOOT", BOOT_0,     BOOT_15,  37, 52,
> -            0,  0,  0,  0,  0, 0,  1, 0,  2, 0),
> -       BANK("C",    GPIOC_0,    GPIOC_7,  53, 60,
> -            1,  0,  1,  0,  3, 0,  4, 0,  5, 0),
> -       BANK("A",    GPIOA_0,    GPIOA_15,  61, 76,
> -            5,  0,  5,  0,  16,  0,  17,  0,  18,  0),
> -       BANK("X",    GPIOX_0,    GPIOX_19,   77, 96,
> -            2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
> +       /* name  first  last  irq  pullen  pull  dir  out  in  ds */
> +       BANK_DS("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
> +               4,  0,  4,  0,  12,  0,  13, 0,  14, 0, 5, 0),
> +       BANK_DS("H",    GPIOH_0,    GPIOH_8, 28, 36,
> +               3,  0,  3,  0,  9,  0,  10,  0,  11,  0, 4, 0),
> +       BANK_DS("BOOT", BOOT_0,     BOOT_15,  37, 52,
> +               0,  0,  0,  0,  0, 0,  1, 0,  2, 0, 0, 0),
> +       BANK_DS("C",    GPIOC_0,    GPIOC_7,  53, 60,
> +               1,  0,  1,  0,  3, 0,  4, 0,  5, 0, 1, 0),
> +       BANK_DS("A",    GPIOA_0,    GPIOA_15,  61, 76,
> +               5,  0,  5,  0,  16,  0,  17,  0,  18,  0, 6, 0),
> +       BANK_DS("X",    GPIOX_0,    GPIOX_19,   77, 96,
> +               2,  0,  2,  0,  6,  0,  7,  0,  8,  0, 2, 0),
>  };
>
>  static struct meson_bank meson_g12a_aobus_banks[] = {
> -       /* name  first  last  irq  pullen  pull  dir  out  in  */
> -       BANK("AO",   GPIOAO_0,  GPIOAO_11,  0, 11,
> -            3,  0,  2, 0,  0,  0,  4, 0,  1,  0),
> +       /* name  first  last  irq  pullen  pull  dir  out  in  ds */
> +       BANK_DS("AO", GPIOAO_0, GPIOAO_11, 0, 11, 3, 0, 2, 0, 0, 0, 4, 0, 1, 0,
> +               0, 0),
>         /* GPIOE actually located in the AO bank */
> -       BANK("E",   GPIOE_0,  GPIOE_2,   97, 99,
> -            3,  16,  2, 16,  0,  16,  4, 16,  1,  16),
> +       BANK_DS("E", GPIOE_0, GPIOE_2, 97, 99, 3, 16, 2, 16, 0, 16, 4, 16, 1,
> +               16, 1, 0),
>  };
these definitions are really hard to read, but it's been like this
even before your patch

>  static struct meson_pmx_bank meson_g12a_periphs_pmx_banks[] = {
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
> index 96a4a72708e4..5108e5aa6514 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
> @@ -174,62 +174,106 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector,
>         return 0;
>  }
>
> -static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
> -                            unsigned long *configs, unsigned num_configs)
> +static int meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
> +                                 enum pin_config_param conf)
can you please confirm that I understood the purpose of this correctly:
I think you introduce this to make setting the bias consistent with
how you set the drive-strength.
if so then it would be great to have a separate patch which describes
that it's only a code-style change and a functional no-op

additionally the function arguments are not consistent with
meson_pinconf_get_drive_strength():
- here you pass the pinctrl subsystem specific parameters (enum
pin_config_param conf)
- in meson_pinconf_get_drive_strength the conversion for pinctrl
subsystem specific values (pinconf_to_config_argument) is part of
meson_pinconf_set
I'm wondering whether two separate functions
(meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make
things easier to read. I haven't tried whether this would really make
things better, so I'd like to hear your opinion on this Guillaume!

[...]
> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
> +                                           unsigned int pin, u16 arg)
>  {
> -       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;
> +       unsigned int ds_val;
> +       int ret;
> +
> +       if (!pc->reg_ds) {
> +               dev_err(pc->dev, "drive-strength not supported\n");
> +               return -ENOTSUPP;
in meson_pinconf_set() we don't complain (with a dev_err) for this case.
I'm not sure what the best-practice is for the pinctrl subsystem,
maybe Linus can comment on this

> +       }
>
>         ret = meson_get_bank(pc, pin, &bank);
>         if (ret)
>                 return ret;
>
> +       meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
> +       bit = bit << 1;
> +
> +       if (arg <= 500) {
> +               ds_val = MESON_PINCONF_DRV_500UA;
> +       } else if (arg <= 2500) {
> +               ds_val = MESON_PINCONF_DRV_2500UA;
> +       } else if (arg <= 3000) {
> +               ds_val = MESON_PINCONF_DRV_3000UA;
> +       } else if (arg <= 4000) {
> +               ds_val = MESON_PINCONF_DRV_4000UA;
> +       } else {
> +               dev_warn_once(pc->dev,
> +                             "pin %u: invalid drive-strength : %d , default to 4mA\n",
> +                             pin, arg);
> +               ds_val = MESON_PINCONF_DRV_4000UA;
why not return -EINVAL here? (my assumption is that the pinctrl
subsystem would like to have -EINVAL instead of drivers doing
fallbacks if the values are out-of-range, but I'm not 100% sure about
this)

[...]
> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
> +                                           unsigned int pin, u16 *arg)
> +{
> +       struct meson_bank *bank;
> +       unsigned int reg, bit;
> +       unsigned int val;
> +       int ret;
> +
do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to
what you already have in meson_pinconf_set_drive_strength()?


Regards
Martin
Martin Blumenstingl April 27, 2019, 7:46 p.m. UTC | #3
Hi Linus,

On Tue, Apr 23, 2019 at 1:13 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
> <glaroque@baylibre.com> 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
>
> This needs to be reviewed by Martin Blumenstigl, Martin can you have a look
> at this patch set? If you don't have the patches I think Guillaume can
> resend them.
thank you for the reminder - I just sent my feedback.
can you please also have a look at patch 4/4? there are two cases
where I'm not sure what the best practice of the pinctrl subsystem is


Regards
Martin
guillaume La Roque April 30, 2019, 7:20 a.m. UTC | #4
Hi Martin,


thanks for your feedback.


On 4/27/19 9:44 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
>> drive-strength-uA is a new feature needed for G12A SoC.
>> the default DS setting after boot is usually 500uA and it is not enough for
>> many functions. We need to be able to set the drive strength to reliably
>> enable things like MMC, I2C, etc ...
>>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> I gave this a go on Meson8m2 (meaning I applied all four patches from
> this series and booted the result on my board):
> [Meson8m2 doesn't support drive strength and still boots without any
> crashes or obvious regressions]
> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
>> ---
>>  drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
>>  drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
>>  drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
> personally I would have split this into two separate patches:
> - one for the generic pinctrl-meson part which adds drive-strength-uA support
> - another patch for enabling this on G12A
>
> if nobody else wants you to split this then it's fine for me as well


why not if i send new series i will do.


>
>>  3 files changed, 163 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-g12a.c b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
>> index d494492e98e9..3475cd7bd2af 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson-g12a.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
>> @@ -1304,28 +1304,28 @@ static struct meson_pmx_func meson_g12a_aobus_functions[] = {
>>  };
>>
>>  static struct meson_bank meson_g12a_periphs_banks[] = {
>> -       /* name  first  last  irq  pullen  pull  dir  out  in */
>> -       BANK("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
>> -            4,  0,  4,  0,  12,  0,  13, 0,  14, 0),
>> -       BANK("H",    GPIOH_0,    GPIOH_8, 28, 36,
>> -            3,  0,  3,  0,  9,  0,  10,  0,  11,  0),
>> -       BANK("BOOT", BOOT_0,     BOOT_15,  37, 52,
>> -            0,  0,  0,  0,  0, 0,  1, 0,  2, 0),
>> -       BANK("C",    GPIOC_0,    GPIOC_7,  53, 60,
>> -            1,  0,  1,  0,  3, 0,  4, 0,  5, 0),
>> -       BANK("A",    GPIOA_0,    GPIOA_15,  61, 76,
>> -            5,  0,  5,  0,  16,  0,  17,  0,  18,  0),
>> -       BANK("X",    GPIOX_0,    GPIOX_19,   77, 96,
>> -            2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
>> +       /* name  first  last  irq  pullen  pull  dir  out  in  ds */
>> +       BANK_DS("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
>> +               4,  0,  4,  0,  12,  0,  13, 0,  14, 0, 5, 0),
>> +       BANK_DS("H",    GPIOH_0,    GPIOH_8, 28, 36,
>> +               3,  0,  3,  0,  9,  0,  10,  0,  11,  0, 4, 0),
>> +       BANK_DS("BOOT", BOOT_0,     BOOT_15,  37, 52,
>> +               0,  0,  0,  0,  0, 0,  1, 0,  2, 0, 0, 0),
>> +       BANK_DS("C",    GPIOC_0,    GPIOC_7,  53, 60,
>> +               1,  0,  1,  0,  3, 0,  4, 0,  5, 0, 1, 0),
>> +       BANK_DS("A",    GPIOA_0,    GPIOA_15,  61, 76,
>> +               5,  0,  5,  0,  16,  0,  17,  0,  18,  0, 6, 0),
>> +       BANK_DS("X",    GPIOX_0,    GPIOX_19,   77, 96,
>> +               2,  0,  2,  0,  6,  0,  7,  0,  8,  0, 2, 0),
>>  };
>>
>>  static struct meson_bank meson_g12a_aobus_banks[] = {
>> -       /* name  first  last  irq  pullen  pull  dir  out  in  */
>> -       BANK("AO",   GPIOAO_0,  GPIOAO_11,  0, 11,
>> -            3,  0,  2, 0,  0,  0,  4, 0,  1,  0),
>> +       /* name  first  last  irq  pullen  pull  dir  out  in  ds */
>> +       BANK_DS("AO", GPIOAO_0, GPIOAO_11, 0, 11, 3, 0, 2, 0, 0, 0, 4, 0, 1, 0,
>> +               0, 0),
>>         /* GPIOE actually located in the AO bank */
>> -       BANK("E",   GPIOE_0,  GPIOE_2,   97, 99,
>> -            3,  16,  2, 16,  0,  16,  4, 16,  1,  16),
>> +       BANK_DS("E", GPIOE_0, GPIOE_2, 97, 99, 3, 16, 2, 16, 0, 16, 4, 16, 1,
>> +               16, 1, 0),
>>  };
> these definitions are really hard to read, but it's been like this
> even before your patch
>
>>  static struct meson_pmx_bank meson_g12a_periphs_pmx_banks[] = {
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
>> index 96a4a72708e4..5108e5aa6514 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
>> @@ -174,62 +174,106 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector,
>>         return 0;
>>  }
>>
>> -static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
>> -                            unsigned long *configs, unsigned num_configs)
>> +static int meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
>> +                                 enum pin_config_param conf)
> can you please confirm that I understood the purpose of this correctly:
> I think you introduce this to make setting the bias consistent with
> how you set the drive-strength.
> if so then it would be great to have a separate patch which describes
> that it's only a code-style change and a functional no-op


you  are right, i will separate this changes.


>
> additionally the function arguments are not consistent with
> meson_pinconf_get_drive_strength():
> - here you pass the pinctrl subsystem specific parameters (enum
> pin_config_param conf)
> - in meson_pinconf_get_drive_strength the conversion for pinctrl
> subsystem specific values (pinconf_to_config_argument) is part of
> meson_pinconf_set


for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it.

if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param.


> I'm wondering whether two separate functions
> (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make
> things easier to read. I haven't tried whether this would really make
> things better, so I'd like to hear your opinion on this Guillaume!


no special opinion on this, if you think it's better for understanding i can separate  set_bias function.


>
> [...]
>> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
>> +                                           unsigned int pin, u16 arg)
>>  {
>> -       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;
>> +       unsigned int ds_val;
>> +       int ret;
>> +
>> +       if (!pc->reg_ds) {
>> +               dev_err(pc->dev, "drive-strength not supported\n");
>> +               return -ENOTSUPP;
> in meson_pinconf_set() we don't complain (with a dev_err) for this case.
> I'm not sure what the best-practice is for the pinctrl subsystem,
> maybe Linus can comment on this
>

this check is to be sure it's possible to set drive stength,

if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error

because something is wrong.


>> +       }
>>
>>         ret = meson_get_bank(pc, pin, &bank);
>>         if (ret)
>>                 return ret;
>>
>> +       meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
>> +       bit = bit << 1;
>> +
>> +       if (arg <= 500) {
>> +               ds_val = MESON_PINCONF_DRV_500UA;
>> +       } else if (arg <= 2500) {
>> +               ds_val = MESON_PINCONF_DRV_2500UA;
>> +       } else if (arg <= 3000) {
>> +               ds_val = MESON_PINCONF_DRV_3000UA;
>> +       } else if (arg <= 4000) {
>> +               ds_val = MESON_PINCONF_DRV_4000UA;
>> +       } else {
>> +               dev_warn_once(pc->dev,
>> +                             "pin %u: invalid drive-strength : %d , default to 4mA\n",
>> +                             pin, arg);
>> +               ds_val = MESON_PINCONF_DRV_4000UA;
> why not return -EINVAL here? (my assumption is that the pinctrl
> subsystem would like to have -EINVAL instead of drivers doing
> fallbacks if the values are out-of-range, but I'm not 100% sure about
> this)
>
> [...]


i choose to set a default value instead of generating an error,

in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default.


>> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
>> +                                           unsigned int pin, u16 *arg)
>> +{
>> +       struct meson_bank *bank;
>> +       unsigned int reg, bit;
>> +       unsigned int val;
>> +       int ret;
>> +
> do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to
> what you already have in meson_pinconf_set_drive_strength()?

depending of linux comment on your feedback on meson_pinconf_set_drive_strength

i will update this part .


>
>
> Regards
> Martin


Regards

Guillaume
Martin Blumenstingl April 30, 2019, 8:28 p.m. UTC | #5
Hi Guillaume,

On Tue, Apr 30, 2019 at 9:20 AM guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> Hi Martin,
>
>
> thanks for your feedback.
>
>
> On 4/27/19 9:44 PM, Martin Blumenstingl wrote:
> > Hi Guillaume,
> >
> > On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
> > <glaroque@baylibre.com> wrote:
> >> drive-strength-uA is a new feature needed for G12A SoC.
> >> the default DS setting after boot is usually 500uA and it is not enough for
> >> many functions. We need to be able to set the drive strength to reliably
> >> enable things like MMC, I2C, etc ...
> >>
> >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> > I gave this a go on Meson8m2 (meaning I applied all four patches from
> > this series and booted the result on my board):
> > [Meson8m2 doesn't support drive strength and still boots without any
> > crashes or obvious regressions]
> > Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >
> >> ---
> >>  drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
> >>  drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
> >>  drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
> > personally I would have split this into two separate patches:
> > - one for the generic pinctrl-meson part which adds drive-strength-uA support
> > - another patch for enabling this on G12A
> >
> > if nobody else wants you to split this then it's fine for me as well
>
>
> why not if i send new series i will do.
great, thank you

[...]
> > additionally the function arguments are not consistent with
> > meson_pinconf_get_drive_strength():
> > - here you pass the pinctrl subsystem specific parameters (enum
> > pin_config_param conf)
> > - in meson_pinconf_get_drive_strength the conversion for pinctrl
> > subsystem specific values (pinconf_to_config_argument) is part of
> > meson_pinconf_set
>
>
> for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it.
>
> if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param.
let me use an example to better explain what I mean.

meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
unsigned long *configs, unsigned num_configs)
-> this uses parameters from the pinctrl subsystem only (struct
pinctrl_dev, configs/pinconf_to_config_argument)

meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin)
meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, unsigned
int pin, u16 drive_strength_ua)
-> these use hardware-specific parameters only (struct meson_pinctrl,
[drive strength in uA])

meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
enum pin_config_param conf)
-> this mixes hardware-specific parameters (struct meson_pinctrl) with
parameters from the pinctrl subsystem (enum pin_config_param conf)

> > I'm wondering whether two separate functions
> > (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make
> > things easier to read. I haven't tried whether this would really make
> > things better, so I'd like to hear your opinion on this Guillaume!
>
>
> no special opinion on this, if you think it's better for understanding i can separate  set_bias function.
this goes with the mixed parameters from my previous comment: if we
stick to the way these "private" functions are defined the "set bias"
function parameters should be hardware/driver specific (in other
words: don't use enum pin_config_param conf)
so my initial idea was to keep the switch/case for enum
pin_config_param in meson_pinconf_set and to have these function
declarations:
- meson_pinconf_disable_bias(struct meson_pinctrl *pc, unsigned int pin)
- meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int
pin, bool pull_up)

and to make it clear: I haven't tested whether this *really* looks
better when fully implemented.
let me know what you think - I'm happy with anything that others will
understand in the end (to make easy to catch potential breakage in a
code-review ;))

>
> >
> > [...]
> >> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
> >> +                                           unsigned int pin, u16 arg)
> >>  {
> >> -       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;
> >> +       unsigned int ds_val;
> >> +       int ret;
> >> +
> >> +       if (!pc->reg_ds) {
> >> +               dev_err(pc->dev, "drive-strength not supported\n");
> >> +               return -ENOTSUPP;
> > in meson_pinconf_set() we don't complain (with a dev_err) for this case.
> > I'm not sure what the best-practice is for the pinctrl subsystem,
> > maybe Linus can comment on this
> >
>
> this check is to be sure it's possible to set drive stength,
>
> if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error
>
> because something is wrong.
OK, I see, there are two different use-cases:
- meson_pinconf_set returns -ENOTSUPP in the "default" case if the
pin_config_param is not supported but no error message
- we don't differentiate between SoCs PIN_CONFIG_DRIVE_STRENGTH_UA is
always delegated from meson_pinconf_set() to
meson_pinconf_set_drive_strength(). you also return -ENOTSUPP and
print an error message

what I meant with my original comment is that there are two different
"-ENOTSUPP" cases but only one prints an error.
I don't know if there are any rules in the pinctrl subsystem how these
cases should be implemented, maybe Linus W. can give his feedback on
this topic.

>
> >> +       }
> >>
> >>         ret = meson_get_bank(pc, pin, &bank);
> >>         if (ret)
> >>                 return ret;
> >>
> >> +       meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
> >> +       bit = bit << 1;
> >> +
> >> +       if (arg <= 500) {
> >> +               ds_val = MESON_PINCONF_DRV_500UA;
> >> +       } else if (arg <= 2500) {
> >> +               ds_val = MESON_PINCONF_DRV_2500UA;
> >> +       } else if (arg <= 3000) {
> >> +               ds_val = MESON_PINCONF_DRV_3000UA;
> >> +       } else if (arg <= 4000) {
> >> +               ds_val = MESON_PINCONF_DRV_4000UA;
> >> +       } else {
> >> +               dev_warn_once(pc->dev,
> >> +                             "pin %u: invalid drive-strength : %d , default to 4mA\n",
> >> +                             pin, arg);
> >> +               ds_val = MESON_PINCONF_DRV_4000UA;
> > why not return -EINVAL here? (my assumption is that the pinctrl
> > subsystem would like to have -EINVAL instead of drivers doing
> > fallbacks if the values are out-of-range, but I'm not 100% sure about
> > this)
> >
> > [...]
>
>
> i choose to set a default value instead of generating an error,
>
> in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default.
in that case I'm fine with it if Linus W. is happy as well (I'm not
sure if there are any rules about "fallback values" in the pinctrl
subsystem)

>
> >> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
> >> +                                           unsigned int pin, u16 *arg)
> >> +{
> >> +       struct meson_bank *bank;
> >> +       unsigned int reg, bit;
> >> +       unsigned int val;
> >> +       int ret;
> >> +
> > do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to
> > what you already have in meson_pinconf_set_drive_strength()?
>
> depending of linux comment on your feedback on meson_pinconf_set_drive_strength
great, thank you!


Martin
guillaume La Roque May 6, 2019, 1:16 p.m. UTC | #6
hi Martin,


On 4/30/19 10:28 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Tue, Apr 30, 2019 at 9:20 AM guillaume La Roque
> <glaroque@baylibre.com> wrote:
>> Hi Martin,
>>
>>
>> thanks for your feedback.
>>
>>
>> On 4/27/19 9:44 PM, Martin Blumenstingl wrote:
>>> Hi Guillaume,
>>>
>>> On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
>>> <glaroque@baylibre.com> wrote:
>>>> drive-strength-uA is a new feature needed for G12A SoC.
>>>> the default DS setting after boot is usually 500uA and it is not enough for
>>>> many functions. We need to be able to set the drive strength to reliably
>>>> enable things like MMC, I2C, etc ...
>>>>
>>>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>>> I gave this a go on Meson8m2 (meaning I applied all four patches from
>>> this series and booted the result on my board):
>>> [Meson8m2 doesn't support drive strength and still boots without any
>>> crashes or obvious regressions]
>>> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>
>>>> ---
>>>>  drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
>>>>  drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
>>>>  drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
>>> personally I would have split this into two separate patches:
>>> - one for the generic pinctrl-meson part which adds drive-strength-uA support
>>> - another patch for enabling this on G12A
>>>
>>> if nobody else wants you to split this then it's fine for me as well
>>
>> why not if i send new series i will do.
> great, thank you
>
> [...]
>>> additionally the function arguments are not consistent with
>>> meson_pinconf_get_drive_strength():
>>> - here you pass the pinctrl subsystem specific parameters (enum
>>> pin_config_param conf)
>>> - in meson_pinconf_get_drive_strength the conversion for pinctrl
>>> subsystem specific values (pinconf_to_config_argument) is part of
>>> meson_pinconf_set
>>
>> for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it.
>>
>> if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param.
> let me use an example to better explain what I mean.
>
> meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
> unsigned long *configs, unsigned num_configs)
> -> this uses parameters from the pinctrl subsystem only (struct
> pinctrl_dev, configs/pinconf_to_config_argument)
>
> meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin)
> meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, unsigned
> int pin, u16 drive_strength_ua)
> -> these use hardware-specific parameters only (struct meson_pinctrl,
> [drive strength in uA])
>
> meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
> enum pin_config_param conf)
> -> this mixes hardware-specific parameters (struct meson_pinctrl) with
> parameters from the pinctrl subsystem (enum pin_config_param conf)


ok i see, it fix in next series.

>
>>> I'm wondering whether two separate functions
>>> (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make
>>> things easier to read. I haven't tried whether this would really make
>>> things better, so I'd like to hear your opinion on this Guillaume!
>>
>> no special opinion on this, if you think it's better for understanding i can separate  set_bias function.
> this goes with the mixed parameters from my previous comment: if we
> stick to the way these "private" functions are defined the "set bias"
> function parameters should be hardware/driver specific (in other
> words: don't use enum pin_config_param conf)
> so my initial idea was to keep the switch/case for enum
> pin_config_param in meson_pinconf_set and to have these function
> declarations:
> - meson_pinconf_disable_bias(struct meson_pinctrl *pc, unsigned int pin)
> - meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int
> pin, bool pull_up)
>
> and to make it clear: I haven't tested whether this *really* looks
> better when fully implemented.
> let me know what you think - I'm happy with anything that others will
> understand in the end (to make easy to catch potential breakage in a
> code-review ;))


what i would like to say is it ok for me, it's better to review.

i will do one patch for enable/disable bias function, one for adding drive-strength function and one for adding G12A part.


>>> [...]
>>>> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
>>>> +                                           unsigned int pin, u16 arg)
>>>>  {
>>>> -       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;
>>>> +       unsigned int ds_val;
>>>> +       int ret;
>>>> +
>>>> +       if (!pc->reg_ds) {
>>>> +               dev_err(pc->dev, "drive-strength not supported\n");
>>>> +               return -ENOTSUPP;
>>> in meson_pinconf_set() we don't complain (with a dev_err) for this case.
>>> I'm not sure what the best-practice is for the pinctrl subsystem,
>>> maybe Linus can comment on this
>>>
>> this check is to be sure it's possible to set drive stength,
>>
>> if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error
>>
>> because something is wrong.
> OK, I see, there are two different use-cases:
> - meson_pinconf_set returns -ENOTSUPP in the "default" case if the
> pin_config_param is not supported but no error message
> - we don't differentiate between SoCs PIN_CONFIG_DRIVE_STRENGTH_UA is
> always delegated from meson_pinconf_set() to
> meson_pinconf_set_drive_strength(). you also return -ENOTSUPP and
> print an error message
>
> what I meant with my original comment is that there are two different
> "-ENOTSUPP" cases but only one prints an error.
> I don't know if there are any rules in the pinctrl subsystem how these
> cases should be implemented, maybe Linus W. can give his feedback on
> this topic.


i just add error message for drive strength because it's an optional feature, people can forget to set register part and just set pin part

so they can see why it's not working.


>>>> +       }
>>>>
>>>>         ret = meson_get_bank(pc, pin, &bank);
>>>>         if (ret)
>>>>                 return ret;
>>>>
>>>> +       meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
>>>> +       bit = bit << 1;
>>>> +
>>>> +       if (arg <= 500) {
>>>> +               ds_val = MESON_PINCONF_DRV_500UA;
>>>> +       } else if (arg <= 2500) {
>>>> +               ds_val = MESON_PINCONF_DRV_2500UA;
>>>> +       } else if (arg <= 3000) {
>>>> +               ds_val = MESON_PINCONF_DRV_3000UA;
>>>> +       } else if (arg <= 4000) {
>>>> +               ds_val = MESON_PINCONF_DRV_4000UA;
>>>> +       } else {
>>>> +               dev_warn_once(pc->dev,
>>>> +                             "pin %u: invalid drive-strength : %d , default to 4mA\n",
>>>> +                             pin, arg);
>>>> +               ds_val = MESON_PINCONF_DRV_4000UA;
>>> why not return -EINVAL here? (my assumption is that the pinctrl
>>> subsystem would like to have -EINVAL instead of drivers doing
>>> fallbacks if the values are out-of-range, but I'm not 100% sure about
>>> this)
>>>
>>> [...]
>>
>> i choose to set a default value instead of generating an error,
>>
>> in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default.
> in that case I'm fine with it if Linus W. is happy as well (I'm not
> sure if there are any rules about "fallback values" in the pinctrl
> subsystem)
>
>>>> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
>>>> +                                           unsigned int pin, u16 *arg)
>>>> +{
>>>> +       struct meson_bank *bank;
>>>> +       unsigned int reg, bit;
>>>> +       unsigned int val;
>>>> +       int ret;
>>>> +
>>> do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to
>>> what you already have in meson_pinconf_set_drive_strength()?
>> depending of linux comment on your feedback on meson_pinconf_set_drive_strength
> great, thank you!
>
>
> Martin


Guillaume