| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGXv+5GnKX+kkYwuqJ2V_1GkgQOfWZgS=MSy5pfiBDo=X4d+mA@mail.gmail.com>
Date: Thu, 17 Oct 2024 16:58:39 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Bo Ye <bo.ye@...iatek.com>
Cc: Sean Wang <sean.wang@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Yongdong Zhang <yongdong.zhang@...iatek.com>, Xiujuan Tan <xiujuan.tan@...iatek.com>,
Browse Zhang <browse.zhang@...iatek.com>, Light Hsieh <light.hsieh@...iatek.com>,
Evan Cao <ot_evan.cao@...iatek.com>, linux-mediatek@...ts.infradead.org,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] pinctrl: mediatek: paris: Revert "Rework support for PIN_CONFIG_{INPUT,OUTPUT}_ENABLE"
On Thu, Oct 17, 2024 at 3:57 PM Bo Ye <bo.ye@...iatek.com> wrote:
>
> [This reverts commit c5d3b64c568a344e998830e0e94a7c04e372f89b.]
>
> For MTK HW,
> 1. to enable GPIO input direction: set DIR=0, IES=1
> 2. to enable GPIO output direction: set DIR=1, and set DO=1 to output high, set DO=0 to out low
>
> The PIN_CONFIG_INPUT/PIN_CONFIG_OUTPUT/PIN_CONFIG_INPUT_ENABLE/PIN_CONFIG_OUTPUT_ENABLE shall
Of note, there is no "PIN_CONFIG_INPUT" option.
> be implemented according to view of its purpose - set GPIO direction and output value (for
> output only) according to specific HW design.
I disagree. The PIN_CONFIG_* options have proper meanings defined in
include/linux/pinctrl/pinconf-generic.h
that every driver implementing the generic pin config API should follow.
For PIN_CONFIG_INPUT_ENABLE: enable the pin's input. Note that this
does not affect the pin's ability to drive output.
Note the latter sentence. When you say set the direction of the GPIO
function, it clearly affects the pin's ability to drive output.
Similar argument for PIN_CONFIG_OUTPUT_ENABLE.
My understanding is that PIN_CONFIG_INPUT_ENABLE and PIN_CONFIG_OUTPUT_ENABLE
are _not_ for setting GPIO function directions. Another angle to think
about it from: if the pin _was not_ muxed to the GPIO function, this
kind of does nothing in terms of the function of the pin. It still
continues passing signals for whatever function it is currently muxed
to.
> However, the reverted patch implement according to author's own explanation of IES without
> understanding of MTK's HW. Such patch does not correctly set DIR/IES bit to control GPIO
> direction on MTK's HW.
Can you provide a correct and detailed explanation then?
MediaTek's datasheet says _nothing_ about the hardware implementation
details or how it's supposed to be configured. There's just a diagram
showing the hardware, which seems to be a closer match to type A as
described in "GPIO mode pitfalls" from Documentation/driver-api/pinctl.rst
Neither does the upstream binding say anything about using the generic
pin config properties with MediaTek pinctrl devices. The original commit
adding this driver mentions "add pinctrl-paris that implements the vendor
dt-bindings", vendor bindings that don't seem to say anything, at least
not the upstream ones.
The generic pin config binding however *does* clearly describe what
each of these properties mean. See
Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
For example:
input-enable:
type: boolean
description: enable input on pin (no effect on output, such as
enabling an input buffer)
And
output-low:
type: boolean
description: set the pin to output mode with low level
If the hardware has some quirks or limitations, such as the SMT bit
can't be set when the GPIO direction is set to output, then that
should absolutely be considered, fixed and *properly documented*.
So what exactly is the breakage you are seeing? What options were
used in what scenario? What is it intending to do?
> Signed-off-by: Light Hsieh <light.hsieh@...iatek.com>
> Signed-off-by: Evan Cao <ot_evan.cao@...iatek.com>
> Signed-off-by: Bo Ye <bo.ye@...iatek.com>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 38 +++++++++++++++++-------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 87e958d827bf..a8af62e6f8ca 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -165,21 +165,20 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
> err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_SR, &ret);
> break;
> case PIN_CONFIG_INPUT_ENABLE:
> - err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_IES, &ret);
> - if (!ret)
> - err = -EINVAL;
> - break;
> - case PIN_CONFIG_OUTPUT:
> + case PIN_CONFIG_OUTPUT_ENABLE:
> err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &ret);
> if (err)
> break;
> + /* CONFIG Current direction return value
> + * ------------- ----------------- ----------------------
> + * OUTPUT_ENABLE output 1 (= HW value)
> + * input 0 (= HW value)
> + * INPUT_ENABLE output 0 (= reverse HW value)
> + * input 1 (= reverse HW value)
> + */
> + if (param == PIN_CONFIG_INPUT_ENABLE)
> + ret = !ret;
>
> - if (!ret) {
> - err = -EINVAL;
> - break;
> - }
> -
> - err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DO, &ret);
> break;
> case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &ret);
> @@ -284,9 +283,26 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> break;
> err = hw->soc->bias_set_combo(hw, desc, 0, arg);
> break;
> + case PIN_CONFIG_OUTPUT_ENABLE:
> + err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_SMT,
> + MTK_DISABLE);
> + /* Keep set direction to consider the case that a GPIO pin
> + * does not have SMT control
> + */
> + if (err != -ENOTSUPP)
> + break;
> +
> + err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DIR,
> + MTK_OUTPUT);
> + break;
Looking now, the device tree bindings don't even allow "output-enable"
or "output-disable" so the PIN_CONFIG_OUTPUT_ENABLE condition won't even
ever get exercised.
So I don't know why this section even exists. At least this part of my
patch was correct.
> case PIN_CONFIG_INPUT_ENABLE:
> /* regard all non-zero value as enable */
> err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_IES, !!arg);
> + if (err)
> + break;
> +
> + err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DIR,
> + MTK_INPUT);
FWIW the implementation is also slightly wrong in that it is ignoring
the argument. If the DT specifies "input-disable", then what happens
is you get PIN_CONFIG_INPUT_ENABLE with "arg = 0", but you still set
it to the input direction.
Thanks
ChenYu
> break;
> case PIN_CONFIG_SLEW_RATE:
> /* regard all non-zero value as enable */
> --
> 2.17.0
>
>
Powered by blists - more mailing lists