Message-ID: <8c9bc4cc-09f1-71db-6386-b486e67d6a2a@collabora.com>
Date:   Wed, 4 May 2022 11:08:58 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>
Cc:     linus.walleij@...aro.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, matthias.bgg@...il.com,
        sean.wang@...nel.org, linux-gpio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com
Subject: Re: [PATCH v2 1/2] dt-bindings: pinctrl: Add MediaTek MT6795 pinctrl
 bindings

Il 04/05/22 02:33, Nícolas F. R. A. Prado ha scritto:
> Hi Angelo,
> 
> On Tue, May 03, 2022 at 04:25:36PM +0200, AngeloGioacchino Del Regno wrote:
>> Add devicetree and pinfunc bindings for MediaTek Helio X10 MT6795.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> ---
>>   .../pinctrl/mediatek,pinctrl-mt6795.yaml      | 224 +++++
>>   include/dt-bindings/pinctrl/mt6795-pinfunc.h  | 908 ++++++++++++++++++
>>   2 files changed, 1132 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/mediatek,pinctrl-mt6795.yaml
>>   create mode 100644 include/dt-bindings/pinctrl/mt6795-pinfunc.h
>>
> 
> ...
> 
>> +
>> +          bias-pull-down:
>> +            oneOf:
>> +              - type: boolean
>> +              - enum: [100, 101, 102, 103]
>> +                description: mt6795 pull down PUPD/R0/R1 type define value.
>> +            description: |
>> +               For normal pull down type, it is not necessary to specify R1R0
>> +               values; When pull down type is PUPD/R0/R1, adding R1R0 defines
>> +               will set different resistance values.
>> +
>> +          bias-pull-up:
>> +            oneOf:
>> +              - type: boolean
>> +              - enum: [100, 101, 102, 103]
>> +                description: mt6795 pull up PUPD/R0/R1 type define value.
>> +            description: |
>> +               For normal pull up type, it is not necessary to specify R1R0
>> +               values; When pull up type is PUPD/R0/R1, adding R1R0 defines
>> +               will set different resistance values.
>> +
>> +          bias-disable: true
>> +
>> +          output-high: true
>> +
>> +          output-low: true
>> +
>> +          input-enable: true
>> +
>> +          input-disable: true
>> +
>> +          input-schmitt-enable: true
>> +
>> +          input-schmitt-disable: true
>> +
>> +          mediatek,pull-up-adv:
>> +            description: |
>> +              Pull up setings for 2 pull resistors, R0 and R1. User can
>> +              configure those special pins. Valid arguments are described as below:
>> +              0: (R1, R0) = (0, 0) which means R1 disabled and R0 disabled.
>> +              1: (R1, R0) = (0, 1) which means R1 disabled and R0 enabled.
>> +              2: (R1, R0) = (1, 0) which means R1 enabled and R0 disabled.
>> +              3: (R1, R0) = (1, 1) which means R1 enabled and R0 enabled.
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +            enum: [0, 1, 2, 3]
>> +
>> +          mediatek,pull-down-adv:
>> +            description: |
>> +              Pull down settings for 2 pull resistors, R0 and R1. User can
>> +              configure those special pins. Valid arguments are described as below:
>> +              0: (R1, R0) = (0, 0) which means R1 disabled and R0 disabled.
>> +              1: (R1, R0) = (0, 1) which means R1 disabled and R0 enabled.
>> +              2: (R1, R0) = (1, 0) which means R1 enabled and R0 disabled.
>> +              3: (R1, R0) = (1, 1) which means R1 enabled and R0 enabled.
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +            enum: [0, 1, 2, 3]
> 
> I'm actually myself trying to figure out why there are two ways of setting
> R0/R1 in the dt-binding (and which should preferred for mt8192 and others):
> 1. passing 0-3 to mediatek,pull-{up,down}-adv
> 2. passing one of the MTK_PUPD_SET_R1R0_** flags to bias-pull-{up,down}
> 
> When the pin is of type MTK_PULL_PUPD_R1R0_TYPE (which should be the only case
> in which it makes sense to consider mediatek,pull-{up,down}-adv AFAIU), they end
> up doing the same thing, it's:
> 
> mtk_pinconf_bias_set_combo() -> mtk_pinconf_bias_set_pupd_r1_r0()
> vs
> mtk_pinconf_adv_pull_set()
> 
> ... and they write to the same registers.
> 
> Unless I'm missing something here.
> 
> Thanks,
> Nícolas

Hey!

Yes you're missing something important :-P

First of all, the flow all depends on the pinctrl driver "type": we have
the common (v1), moore and paris.
I will leave this research to you, or this reply will become a wall of text,
so let's go on with the latter details, which are relative to the paris one.

You should check pinctrl/mediatek/pinctrl-mt(model).c: here, you declare a
struct mtk_pin_soc containing pointers to pins data and function callbacks,
such as bias_{get,set}_combo() and adv_pull_{set,get}().


Now to the real deal, examples below:

- Declaring property "mediatek,pull-up-adv" means that you get through
   MTK_PIN_CONFIG_PU_ADV case, which calls the adv_pull_set() callback;
- Declaring property "bias-pull-up", however, means that you get through
   PIN_CONFIG_BIAS_PULL_UP, which calls bias_set_combo().

For adv_pull_set() callback being mtk_pinconf_adv_pull_set() the following happens:
- Write to registers PIN_REG_R0, PIN_REG_R1 (set resistor 0,1 en/disabled);
- Write to registers PIN_REG_PUPD (pull up, or down)
- If below writes fail (because no r0r1/pupd declared in your driver's table),
   the code will call the bias_set() callback as a fallback.

For bias_set_combo() cb being mtk_pinconf_bias_set_combo() the following happens:
- Check pull_type mask, it may be one or a combination (OR) of:
   - MTK_PULL_RSEL_TYPE
   - MTK_PULL_PU_PD_TYPE
   - MTK_PULL_PULLSEL_TYPE
   - MTK_PULL_PUPD_R1R0_TYPE
- Call function(s!) mtk_pinconf_bias_set_{rsel,pu_pd,pullsel_pullen,pupd_r1_r0},
   depending on the bits set in the pull_type mask.

All of the aforementioned functions will perform a different register write for
different pullup/pulldown settings..

bias-pull-up:
One of the cases in which you want to use the combo is when you need to set a
combination of, let's say, PULLSEL and R1R0 (enabling a default pull, adding a
series resistance of parallel R1R0 resistors to the default value), which is
similar to RSEL_TYPE, but for SoCs that don't have the RSEL_TYPE register layout.

mediatek,pull-up-adv:
One of the cases in which you want to use the adv_pull_set is when you want to
set *only* R1R0+PUPD or *only* PULLSEL (when r1r0/pupd not supported), but also
*never* modify PU_PD or RSEL registers.


It's a bit tricky to understand, but this stuff is really on a per-SoC basis, as
not all of them will behave like the other... so if you check only pinctrl-paris,
or only pinctrl-mtk-common-v2, you will inevitably go offroad with your research.
The MediaTek pinctrl mutates behavior in a combination of the aforementioned two
*and* the SoC-specific pinctrl data (pinctrl-mt6795.c).

Though, maybe-and-I-say-maybe (because I haven't performed a *full* research on
that topic), it *may be* possible to refactor the mtk pinctrl framework to get
rid of the mediatek,pull-up-adv property and do it all from the standard pinconf
bias-pull-up property instead with some driver magic... but.. in any case, that's
for another time - definitely not now (for me, at least - but if anyone wants to
explore this territory and produce more cleanups, you're encouraged to).


Please note that I tried to simplify the explanation as much as possible and
something inside may have to be "interpreted the right way", when looking for
details that are way more advanced compared to the basics that are explained
here... but you see, it's already a big wall of text... :-) :-)

Cheers,
Angelo

Powered by blists - more mailing lists