Message-ID: <66218a3c-04c5-42ea-ba9c-e0fbc72ed16f@collabora.com>
Date: Mon, 13 Jan 2025 14:20:54 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, Cathy Xu <ot_cathy.xu@...iatek.com>
Cc: Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
 Sean Wang <sean.wang@...nel.org>, Lei Xue <lei.xue@...iatek.com>,
 wenbin.mei@...iatek.com, linux-gpio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 Guodong Liu <guodong.liu@...iatek.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: pinctrl: mediatek: add support for
 mt8196

Il 11/01/25 10:41, Krzysztof Kozlowski ha scritto:
> On Fri, Jan 10, 2025 at 06:42:28PM +0800, Cathy Xu wrote:
>> 1.Add pinctrl file on MediaTek mt8196.
> 
> Where? What is pinctrl file?
> 
>> 2.Add the new binding document for pinctrl on MediaTek mt8196.
> 
> Look at git history how commit msgs are written for such changes.
> 
>>
>> Signed-off-by: Guodong Liu <guodong.liu@...iatek.com>
>> Signed-off-by: Cathy Xu <ot_cathy.xu@...iatek.com>
>> ---
>>   .../pinctrl/mediatek,mt8196-pinctrl.yaml      |  266 +++
>>   include/dt-bindings/pinctrl/mt8196-pinfunc.h  | 1572 +++++++++++++++++
>>   2 files changed, 1838 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/mediatek,mt8196-pinctrl.yaml
>>   create mode 100644 include/dt-bindings/pinctrl/mt8196-pinfunc.h
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt8196-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt8196-pinctrl.yaml
>> new file mode 100644
>> index 000000000000..abeb0d942cc4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt8196-pinctrl.yaml
>> @@ -0,0 +1,266 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/mediatek,mt8196-pinctrl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek MT8196 Pin Controller
>> +
>> +maintainers:
>> +  - Lei Xue <lei.xue@...iatek.com>
>> +  - Cathy Xu <ot_cathy.xu@...iatek.com>
>> +
>> +description:
>> +  The MediaTek's MT8196 Pin controller is used to control SoC pins.
>> +
>> +properties:
>> +  compatible:
>> +    const: mediatek,mt8196-pinctrl
>> +
>> +  gpio-controller: true
>> +
>> +  '#gpio-cells':
>> +    description:
>> +      Number of cells in GPIO specifier, should be two. The first cell is the
>> +      pin number, the second cell is used to specify optional parameters which
>> +      are defined in <dt-bindings/gpio/gpio.h>.
>> +    const: 2
>> +
>> +  gpio-ranges:
>> +    maxItems: 1
>> +
>> +  gpio-line-names: true
>> +
>> +  reg:
>> +    items:
>> +      - description: gpio registers base address
>> +      - description: rt group io configuration registers base address
>> +      - description: rm1 group io configuration registers base address
>> +      - description: rm2 group io configuration registers base address
>> +      - description: rb group io configuration registers base address
>> +      - description: bm1 group io configuration registers base address
>> +      - description: bm2 group io configuration registers base address
>> +      - description: bm3 group io configuration registers base address
>> +      - description: lt group io configuration registers base address
>> +      - description: lm1 group io configuration registers base address
>> +      - description: lm2 group io configuration registers base address
>> +      - description: lb1 group io configuration registers base address
>> +      - description: lb2 group io configuration registers base address
>> +      - description: tm1 group io configuration registers base address
>> +      - description: tm2 group io configuration registers base address
>> +      - description: tm3 group io configuration registers base address
>> +
>> +  reg-names:
>> +    items:
>> +      - const: iocfg0
>> +      - const: iocfg_rt
>> +      - const: iocfg_rm1
>> +      - const: iocfg_rm2
>> +      - const: iocfg_rb
>> +      - const: iocfg_bm1
>> +      - const: iocfg_bm2
>> +      - const: iocfg_bm3
>> +      - const: iocfg_lt
>> +      - const: iocfg_lm1
>> +      - const: iocfg_lm2
>> +      - const: iocfg_lb1
>> +      - const: iocfg_lb2
>> +      - const: iocfg_tm1
>> +      - const: iocfg_tm2
>> +      - const: iocfg_tm3
> 
> Are you sure these are separate address spaces?
> 

Those are different partitions of the GPIO controller, of which, each one does
provide full control and different functions that are partition-global and not
controller-global.

So, I can confirm that these are indeed separate address spaces - this is right.

>> +
>> +  interrupt-controller: true
>> +
>> +  '#interrupt-cells':
>> +    const: 2
>> +
>> +  interrupts:
>> +    description: The interrupt outputs to sysirq.
>> +    maxItems: 1
>> +
>> +  mediatek,rsel-resistance-in-si-unit:
>> +    type: boolean
>> +    description:
>> +      We provide two methods to select the resistance for I2C when pull up or
>> +      pull down. The first is by RSEL definition value, another one is by
>> +      resistance value(ohm). This flag is used to identify if the method is
>> +      resistance(si unit) value.
> 
> What is the point of choosing it? This is one hardware, one SoC, so how
> different boards can have different units? No, just use Ohms
> 

That's legacy from when the paris driver didn't specify the RSEL in ohms, and I
agree about deprecating that property and making it a default.

Cathy, please add a `has_legacy_rsel` boolean in the platform data and assign that
to true in *all* MediaTek pinctrl drivers that are not MT8196, then in
pinctrl-paris:

if (hw->soc->has_legacy_rsel)
	hw->rsel_si_unit = of_property_read_bool(....)
else
	hw->rsel_si_unit = true;

That way we can finally drop this property from drivers for new SoCs, including
the one for MT8196.

>> +
>> +# PIN CONFIGURATION NODES
>> +patternProperties:
>> +  '-pins$':
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    patternProperties:
>> +      '^pins':
>> +        type: object
>> +        $ref: /schemas/pinctrl/pincfg-node.yaml
>> +        additionalProperties: false
>> +        description:
>> +          A pinctrl node should contain at least one subnode representing the
>> +          pinctrl groups available on the machine. Each subnode will list the
>> +          pins it needs, and how they should be configured, with regard to muxer
>> +          configuration, pullups, drive strength, input enable/disable and input
>> +          schmitt.
>> +
>> +        properties:
>> +          pinmux:
>> +            description:
>> +              Integer array, represents gpio pin number and mux setting.
>> +              Supported pin number and mux varies for different SoCs, and are
>> +              defined as macros in dt-bindings/pinctrl/mt8196-pinfunc.h
>> +              directly, for this SoC.
>> +
>> +          drive-strength:
>> +            enum: [2, 4, 6, 8, 10, 12, 14, 16]
>> +
>> +          drive-strength-microamp:
>> +            enum: [125, 250, 500, 1000]
> 
> Why duplicating properties? No, use only one.
> 

The problem here is not entirely about duplicating properties, and I'm not
sure that the reason is actually acceptable (but being this a special case
the `description` field would be mandatory to have IMO!!).

So, the reason for this separation is that the drive-strength-microamp does
activate a special feature in the controller called "advanced drive strength
mode", which is switching to different shunts that will decrease the power
efficiency of the chip (by an ignorable amount, if that's one pin - but if
that goes to something like 100 pins, it's not ignorable anymore).

I'd be happy if we could let them retain both properties after putting a
clear description of what's happening and why there are two of them.

>> +
>> +          bias-pull-down:
>> +            oneOf:
>> +              - type: boolean
>> +              - enum: [100, 101, 102, 103]
>> +                description: mt8196 pull down PUPD/R0/R1 type define value.
>> +              - enum: [200, 201, 202, 203, 204, 205, 206, 207]
>> +                description: mt8196 pull down RSEL type define value.

These two must go away, RSEL shall be defined in Ohms unit in DTs/bindings.

>> +              - enum: [75000, 5000]
>> +                description: mt8196 pull down RSEL type si unit value(ohm).
>> +            description: |
>> +              For pull down type is normal, it doesn't need add RSEL & R1R0
>> +              define and resistance value.
>> +              For pull down type is PUPD/R0/R1 type, it can add R1R0 define to
>> +              set different resistance. It can support "MTK_PUPD_SET_R1R0_00" &
>> +              "MTK_PUPD_SET_R1R0_01" & "MTK_PUPD_SET_R1R0_10" &
>> +              "MTK_PUPD_SET_R1R0_11" define in mt8196.
>> +              For pull down type is RSEL, it can add RSEL define & resistance
>> +              value(ohm) to set different resistance by identifying property
>> +              "mediatek,rsel-resistance-in-si-unit". It can support
>> +              "MTK_PULL_SET_RSEL_000" & "MTK_PULL_SET_RSEL_001" &
>> +              "MTK_PULL_SET_RSEL_010" & "MTK_PULL_SET_RSEL_011" &
>> +              "MTK_PULL_SET_RSEL_100" & "MTK_PULL_SET_RSEL_101" &
>> +              "MTK_PULL_SET_RSEL_110" & "MTK_PULL_SET_RSEL_111" define in
>> +              mt8196. It can also support resistance value(ohm) "75000" & "5000"
>> +              in mt8196.
>> +
>> +          bias-pull-up:
>> +            oneOf:
>> +              - type: boolean
>> +              - enum: [100, 101, 102, 103]
>> +                description: mt8196 pull up PUPD/R0/R1 type define value.
>> +              - enum: [200, 201, 202, 203, 204, 205, 206, 207]
>> +                description: mt8196 pull up RSEL type define value.
>> +              - enum: [1000, 1500, 2000, 3000, 4000, 5000, 10000, 75000]
>> +                description: mt8196 pull up RSEL type si unit value(ohm).
> 
> Same problems.
> 
>> +++ b/include/dt-bindings/pinctrl/mt8196-pinfunc.h
>> @@ -0,0 +1,1572 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2025 Mediatek Inc.
>> + * Author: Guodong Liu <Guodong.Liu@...iatek.com>
>> + */
>> +
>> +#ifndef __MT8196_PINFUNC_H
>> +#define __MT8196_PINFUNC_H
>> +
>> +#include <dt-bindings/pinctrl/mt65xx.h>
>> +
>> +#define PINMUX_GPIO0__FUNC_GPIO0 (MTK_PIN_NO(0) | 0)
>> +#define PINMUX_GPIO0__FUNC_DMIC1_CLK (MTK_PIN_NO(0) | 1)
>> +#define PINMUX_GPIO0__FUNC_SPI3_A_MO (MTK_PIN_NO(0) | 3)
>> +#define PINMUX_GPIO0__FUNC_FMI2S_B_LRCK (MTK_PIN_NO(0) | 4)
>> +#define PINMUX_GPIO0__FUNC_SCP_DMIC1_CLK (MTK_PIN_NO(0) | 5)
>> +#define PINMUX_GPIO0__FUNC_TP_GPIO14_AO (MTK_PIN_NO(0) | 6)
> 
> You got comment, so respond to it. Sending the same and expecting
> different results is fast way to get a grumpy response.
> 

...and I agree about every other comment from Krzysztof.

Cheers,
Angelo

Powered by blists - more mailing lists