From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0EF07C47258 for ; Wed, 17 Jan 2024 20:43:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=+NaX+CKYWL8B/VjMVEPVsRanFey6ZI05TZW4itlXwzs=; b=WNosmm5ft/Ofyj +6IvQ81e8jBH73BKzigD5N12SA8rXJcdI9PlL2snq6VdU088chhqqfJz/3EtB47F0vWkB5ojP7cg/ 8n3NoYAzYfk3XeTXvLkyMv69nVlnTFH4xxfqbTn5iMEWBHzchZP628m+e8xqMHCL00sbNweLSd+ce OSHEhh0ZG94viuOSP9Av4hGECvaCMLbd21TnRwu0kXKEaClRA7GcaPL6iKCVgpmGDV4yCKJHMu71G WkDinXaOJdVRHHnQygS11Okos6TKcSgj0o9mtM/bJSELAG4Wy56/HuHDM7kqp2k3/RRPNSGHldigK MC3NOvgQ9HKRQ0iqo7oQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rQCkt-000gzB-1m; Wed, 17 Jan 2024 20:43:23 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rQCkp-000gyX-05 for linux-arm-kernel@lists.infradead.org; Wed, 17 Jan 2024 20:43:21 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6258311FB; Wed, 17 Jan 2024 12:43:59 -0800 (PST) Received: from pluto (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 025B53F766; Wed, 17 Jan 2024 12:43:10 -0800 (PST) Date: Wed, 17 Jan 2024 20:43:08 +0000 From: Cristian Marussi To: "Peng Fan (OSS)" Cc: Sudeep Holla , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Oleksii Moisieiev , Linus Walleij , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-gpio@vger.kernel.org, AKASHI Takahiro , Peng Fan Subject: Re: [PATCH v2 4/6] pinctrl: Implementation of the generic scmi-pinctrl driver Message-ID: References: <20240104-pinctrl-scmi-v2-0-a9bd86ab5a84@nxp.com> <20240104-pinctrl-scmi-v2-4-a9bd86ab5a84@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240104-pinctrl-scmi-v2-4-a9bd86ab5a84@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240117_124319_184933_AB01333D X-CRM114-Status: GOOD ( 37.43 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Jan 04, 2024 at 06:48:48PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan > > scmi-pinctrl driver implements pinctrl driver interface and using > SCMI protocol to redirect messages from pinctrl subsystem SDK to > SCMI platform firmware, which does the changes in HW. > Hi, a few remarks. > Co-developed-by: Oleksii Moisieiev > Signed-off-by: Oleksii Moisieiev > Signed-off-by: Peng Fan > --- > MAINTAINERS | 1 + > drivers/pinctrl/Kconfig | 11 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-scmi.c | 524 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 537 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 487bff0d44c0..3fe790e90834 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -21314,6 +21314,7 @@ F: drivers/clk/clk-sc[mp]i.c > F: drivers/cpufreq/sc[mp]i-cpufreq.c > F: drivers/firmware/arm_scmi/ > F: drivers/firmware/arm_scpi.c > +F: drivers/pinctrl/pinctrl-scmi.c > F: drivers/pmdomain/arm/ > F: drivers/powercap/arm_scmi_powercap.c > F: drivers/regulator/scmi-regulator.c > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 8163a5983166..27b26e428f60 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -432,6 +432,17 @@ config PINCTRL_ROCKCHIP > help > This support pinctrl and GPIO driver for Rockchip SoCs. > > +config PINCTRL_SCMI > + tristate "Pinctrl driver using SCMI protocol interface" > + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST > + select PINMUX > + select GENERIC_PINCONF > + help > + This driver provides support for pinctrl which is controlled > + by firmware that implements the SCMI interface. > + It uses SCMI Message Protocol to interact with the > + firmware providing all the pinctrl controls. > + > config PINCTRL_SINGLE > tristate "One-register-per-pin type device tree based pinctrl driver" > depends on OF > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index 1071f301cc70..ba755ed2d46c 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -44,6 +44,7 @@ obj-$(CONFIG_PINCTRL_PIC32) += pinctrl-pic32.o > obj-$(CONFIG_PINCTRL_PISTACHIO) += pinctrl-pistachio.o > obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o > obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o > +obj-$(CONFIG_PINCTRL_SCMI) += pinctrl-scmi.o > obj-$(CONFIG_PINCTRL_SINGLE) += pinctrl-single.o > obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o > obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o > diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c > new file mode 100644 > index 000000000000..146308d27f54 > --- /dev/null > +++ b/drivers/pinctrl/pinctrl-scmi.c > @@ -0,0 +1,524 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * System Control and Power Interface (SCMI) Protocol based pinctrl driver > + * > + * Copyright (C) 2023 EPAM > + * Copyright 2023 NXP > + */ 2023-2024 > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#include "pinctrl-utils.h" > +#include "core.h" > +#include "pinconf.h" > + > +#define DRV_NAME "scmi-pinctrl" [snip] > +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = { > + .request = pinctrl_scmi_request, > + .free = pinctrl_scmi_free, > + .get_functions_count = pinctrl_scmi_get_functions_count, > + .get_function_name = pinctrl_scmi_get_function_name, > + .get_function_groups = pinctrl_scmi_get_function_groups, > + .set_mux = pinctrl_scmi_func_set_mux, > +}; > + > +static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param, u8 *type) > +{ Same as in the previous patch, is it reasonable to use the new enum here instead of a generic u8 * ? > + u8 arg = param; > + > + switch (arg) { > + case PIN_CONFIG_BIAS_BUS_HOLD: > + *type = SCMI_PIN_BIAS_BUS_HOLD; > + break; > + case PIN_CONFIG_BIAS_DISABLE: > + *type = SCMI_PIN_BIAS_DISABLE; > + break; > + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE: > + *type = SCMI_PIN_BIAS_HIGH_IMPEDANCE; > + break; > + case PIN_CONFIG_BIAS_PULL_DOWN: > + *type = SCMI_PIN_BIAS_PULL_DOWN; > + break; > + case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT: > + *type = SCMI_PIN_BIAS_PULL_DEFAULT; > + break; > + case PIN_CONFIG_BIAS_PULL_UP: > + *type = SCMI_PIN_BIAS_PULL_UP; > + break; > + case PIN_CONFIG_DRIVE_OPEN_DRAIN: > + *type = SCMI_PIN_DRIVE_OPEN_DRAIN; > + break; > + case PIN_CONFIG_DRIVE_OPEN_SOURCE: > + *type = SCMI_PIN_DRIVE_OPEN_SOURCE; > + break; > + case PIN_CONFIG_DRIVE_PUSH_PULL: > + *type = SCMI_PIN_DRIVE_PUSH_PULL; > + break; > + case PIN_CONFIG_DRIVE_STRENGTH: > + *type = SCMI_PIN_DRIVE_STRENGTH; > + break; > + case PIN_CONFIG_DRIVE_STRENGTH_UA: > + *type = SCMI_PIN_DRIVE_STRENGTH; > + break; > + case PIN_CONFIG_INPUT_DEBOUNCE: > + *type = SCMI_PIN_INPUT_DEBOUNCE; > + break; > + case PIN_CONFIG_INPUT_ENABLE: > + *type = SCMI_PIN_INPUT_MODE; > + break; > + case PIN_CONFIG_INPUT_SCHMITT: > + *type = SCMI_PIN_INPUT_SCHMITT; > + break; > + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: > + *type = SCMI_PIN_INPUT_MODE; > + break; > + case PIN_CONFIG_MODE_LOW_POWER: > + *type = SCMI_PIN_LOW_POWER_MODE; > + break; > + case PIN_CONFIG_OUTPUT: > + *type = SCMI_PIN_OUTPUT_VALUE; > + break; > + case PIN_CONFIG_OUTPUT_ENABLE: > + *type = SCMI_PIN_OUTPUT_MODE; > + break; > + case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: > + *type = SCMI_PIN_OUTPUT_VALUE; > + break; > + case PIN_CONFIG_POWER_SOURCE: > + *type = SCMI_PIN_POWER_SOURCE; > + break; > + case PIN_CONFIG_SLEW_RATE: > + *type = SCMI_PIN_SLEW_RATE; > + break; > + case SCMI_PIN_OEM_START ... SCMI_PIN_OEM_END: > + *type = param; > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev, unsigned int _pin, > + unsigned long *config) > +{ > + int ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param config_type; > + u32 config_value; > + u8 type; > + > + if (!config) > + return -EINVAL; > + > + config_type = pinconf_to_config_param(*config); > + > + ret = pinctrl_scmi_map_pinconf_type(config_type, &type); > + if (ret) { > + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret); > + return ret; > + } > + Issuing a cat /sys/kernel/debug/pictrl//pinconf-pins got a string of (adding some debug to print config_type): [ 89.843868] scmi-pinctrl scmi_dev.12: Error map pinconf_type 22 - ret:-95 [ 89.846125] scmi-pinctrl scmi_dev.12: Error map pinconf_type 23 - ret:-95 but I suppose is due to some missing matching definitions in the SCMI spec....just a heads up for the future. > + ret = pinctrl_ops->config_get(pmx->ph, _pin, PIN_TYPE, type, &config_value); > + if (ret) > + return ret; > + > + *config = pinconf_to_config_packed(config_type, config_value); > + > + return 0; > +} > + > +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev, > + unsigned int _pin, > + unsigned long *configs, > + unsigned int num_configs) > +{ > + int i, ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!configs || !num_configs) > + return -EINVAL; > + > + for (i = 0; i < num_configs; i++) { > + ret = pinctrl_scmi_map_pinconf_type(pinconf_to_config_param(configs[i]), > + &pmx->config_type[i]); > + if (ret) { > + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret); > + return ret; > + } > + pmx->config_value[i] = pinconf_to_config_argument(configs[i]); > + } > + Are you sure that this pinctrl function cannot be called multiple times concurrently ? Because pmx->config_type/value are globally defined for the driver so any concurrent call will lead to corruption. Moreover, you allocate those pmx->config_ arrays in probe as SZ_4K, but effectively how many of these are effectively used in reality ? I mean, we clearly have to address the worst case, which is 256 values by the SCMI spec CONFIG_SET definitions, but typically how many configs are going to be sent in a call ? because if it is a reasonable small values you could just put in on the stack here with a check for overlflow and a dynamic allocation to address the special case...this would also solve the potential concurrent calls issue described above. ...but I could be missing something about Pinctrl so please explain. > + ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, num_configs, > + pmx->config_type, pmx->config_value); > + if (ret) > + dev_err(pmx->dev, "Error parsing config %d\n", ret); > + > + return ret; > +} > + > +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev, > + unsigned int group, > + unsigned long *configs, > + unsigned int num_configs) > +{ > + int i, ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + > + if (!configs || !num_configs) > + return -EINVAL; > + > + for (i = 0; i < num_configs; i++) { > + ret = pinctrl_scmi_map_pinconf_type(pinconf_to_config_param(configs[i]), > + &pmx->config_type[i]); > + if (ret) { > + dev_err(pmx->dev, "Error map pinconf_type %d\n", ret); > + return ret; > + } > + pmx->config_value[i] = pinconf_to_config_argument(configs[i]); > + } > + > + ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, num_configs, > + pmx->config_type, pmx->config_value); > + if (ret) > + dev_err(pmx->dev, "Error parsing config %d", ret); > + > + return ret; > +}; > + > +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev, > + unsigned int group, > + unsigned long *config) > +{ > + int ret; > + struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev); > + enum pin_config_param config_type; > + u32 config_value; > + u8 type; > + > + if (!config) > + return -EINVAL; > + > + config_type = pinconf_to_config_param(*config); > + ret = pinctrl_scmi_map_pinconf_type(config_type, &type); You dont check ret here ? > + > + ret = pinctrl_ops->config_get(pmx->ph, group, GROUP_TYPE, type, &config_value); > + if (ret) > + return ret; > + > + *config = pinconf_to_config_packed(config_type, config_value); > + > + return 0; > +} > + Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel