| Message ID | 1615969516-87663-4-git-send-email-lakshmi.sai.krishna.potthuri@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/3] firmware: xilinx: Add pinctrl support | expand |
Hi Andy Shevchenko, Thanks for the review. > -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Wednesday, March 17, 2021 6:26 PM > To: Sai Krishna Potthuri <lakshmis@xilinx.com> > Cc: Linus Walleij <linus.walleij@linaro.org>; Rob Herring > <robh+dt@kernel.org>; Michal Simek <michals@xilinx.com>; Greg Kroah- > Hartman <gregkh@linuxfoundation.org>; linux-arm Mailing List <linux-arm- > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; devicetree <devicetree@vger.kernel.org>; open > list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>; git <git@xilinx.com>; > saikrishna12468@gmail.com > Subject: Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support > > On Wed, Mar 17, 2021 at 10:27 AM Sai Krishna Potthuri > <lakshmi.sai.krishna.potthuri@xilinx.com> wrote: > > > > Adding pinctrl driver for Xilinx ZynqMP platform. > > This driver queries pin information from firmware and registers pin > > control accordingly. > > > > Signed-off-by: Sai Krishna Potthuri > > <lakshmi.sai.krishna.potthuri@xilinx.com> > > --- > > drivers/pinctrl/Kconfig | 13 + > > drivers/pinctrl/Makefile | 1 + > > drivers/pinctrl/pinctrl-zynqmp.c | 1030 > > ++++++++++++++++++++++++++++++ > > 3 files changed, 1044 insertions(+) > > create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c > > > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index > > 815095326e2d..25d3c7208975 100644 > > --- a/drivers/pinctrl/Kconfig > > +++ b/drivers/pinctrl/Kconfig > > @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ > > help > > This selects the pinctrl driver for Xilinx Zynq. > > > > +config PINCTRL_ZYNQMP > > + bool "Pinctrl driver for Xilinx ZynqMP" > > Why not module? As most of the Xilinx drivers depending on the pin controller driver for configuring the MIO pins, we are not supporting to build this driver as a module. > > > + depends on ARCH_ZYNQMP > > + select PINMUX > > + select GENERIC_PINCONF > > + help > > + This selects the pinctrl driver for Xilinx ZynqMP platform. > > + This driver will query the pin information from the firmware > > + and allow configuring the pins. > > + Configuration can include the mux function to select on those > > + pin(s)/group(s), and various pin configuration parameters > > + such as pull-up, slew rate, etc. > > + > > config PINCTRL_INGENIC > > bool "Pinctrl driver for the Ingenic JZ47xx SoCs" > > default MACH_INGENIC > > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index > > f53933b2ff02..7e058739f0d5 100644 > > --- a/drivers/pinctrl/Makefile > > +++ b/drivers/pinctrl/Makefile > > @@ -43,6 +43,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o > > obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o > > obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o > > obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o > > +obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o > > obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o > > obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o > > obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o > > diff --git a/drivers/pinctrl/pinctrl-zynqmp.c > > b/drivers/pinctrl/pinctrl-zynqmp.c > > new file mode 100644 > > index 000000000000..63edde400e85 > > --- /dev/null > > +++ b/drivers/pinctrl/pinctrl-zynqmp.c > > @@ -0,0 +1,1030 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * ZynqMP pin controller > > + * > > + * Copyright (C) 2020 Xilinx, Inc. > > + * > > + * Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> > > + * Rajan Vaja <rajanv@xilinx.com> > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/of_address.h> > > > +#include <linux/pinctrl/pinmux.h> > > +#include <linux/pinctrl/pinconf-generic.h> > > Can you move this group of headers after linux/* ? > > > +#include <dt-bindings/pinctrl/pinctrl-zynqmp.h> > > Can it be moved outside of these headers > > > +#include <linux/platform_device.h> > > +#include <linux/firmware/xlnx-zynqmp.h> > > Ordering (for all groups of headers)? Ok, I will order the headers in the below order #include <linux/*> #include <linux/firmware/xlnx-zynqmp.h> #include <linux/pinctrl/*> #include <dt-bindings/pinctrl/pinctrl-zynqmp.h> > > > +#include "core.h" > > +#include "pinctrl-utils.h" > > + > > +#define ZYNQMP_PIN_PREFIX "MIO" > > +#define PINCTRL_GET_FUNC_NAME_RESP_LEN 16 > > +#define MAX_FUNC_NAME_LEN 16 > > +#define MAX_GROUP_PIN 50 > > +#define END_OF_FUNCTIONS "END_OF_FUNCTIONS" > > +#define NUM_GROUPS_PER_RESP 6 > > + > > +#define PINCTRL_GET_FUNC_GROUPS_RESP_LEN 12 > > +#define PINCTRL_GET_PIN_GROUPS_RESP_LEN 12 > > +#define NA_GROUP -1 > > +#define RESERVED_GROUP -2 > > + > > +#define DRIVE_STRENGTH_2MA 2 > > +#define DRIVE_STRENGTH_4MA 4 > > +#define DRIVE_STRENGTH_8MA 8 > > +#define DRIVE_STRENGTH_12MA 12 > > + > > +/** > > + * struct zynqmp_pmux_function - a pinmux function > > + * @name: Name of the pinmux function > > + * @groups: List of pingroups for this function > > + * @ngroups: Number of entries in @groups > > > + * @node:` Firmware node matching with for function > > Typo > > > + * > > + * This structure holds information about pin control function > > + * and function group names supporting that function. > > + */ > > +struct zynqmp_pmux_function { > > + char name[MAX_FUNC_NAME_LEN]; > > + const char * const *groups; > > + unsigned int ngroups; > > +}; > > + > > +/** > > + * struct zynqmp_pinctrl - driver data > > + * @pctrl: Pinctrl device > > Pin control > > > + * @groups: Pingroups > > Pin groups > > > + * @ngroups: Number of @groups > > + * @funcs: Pinmux functions > > Pin mux functions I will fix all of them in the next version. > > > + * @nfuncs: Number of @funcs > > + * > > + * This struct is stored as driver data and used to retrieve > > + * information regarding pin control functions, groups and > > + * group pins. > > + */ > > +struct zynqmp_pinctrl { > > + struct pinctrl_dev *pctrl; > > + const struct zynqmp_pctrl_group *groups; > > + unsigned int ngroups; > > + const struct zynqmp_pmux_function *funcs; > > + unsigned int nfuncs; > > +}; > > + > > +/** > > + * struct zynqmp_pctrl_group - Pin control group info > > + * @name: Group name > > + * @pins: Group pin numbers > > + * @npins: Number of pins in group > > + */ > > +struct zynqmp_pctrl_group { > > + const char *name; > > + unsigned int pins[MAX_GROUP_PIN]; > > + unsigned int npins; > > +}; > > + > > +/** > > + * enum zynqmp_pin_config_param - possible pin configuration > parameters > > + * @PIN_CONFIG_IOSTANDARD: if the pin can select an IO standard, > > + * the argument to this parameter (on a > > + * custom format) tells the driver which > > + * alternative IO standard to use > > + */ > > +enum zynqmp_pin_config_param { > > + PIN_CONFIG_IOSTANDARD = PIN_CONFIG_END + 1, }; > > I'm lost here. What is IO standard exactly? Why it can't be in generic > headers? It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts. Since this is specific to Xilinx ZynqMP platform, considered to be added in the driver file. > > > +static const struct pinconf_generic_params zynqmp_dt_params[] = { > > + {"io-standard", PIN_CONFIG_IOSTANDARD, > IO_STANDARD_LVCMOS18}, > > +}; > > + > > +#ifdef CONFIG_DEBUG_FS > > +static const struct > > +pin_config_item zynqmp_conf_items[ARRAY_SIZE(zynqmp_dt_params)] = > { > > + PCONFDUMP(PIN_CONFIG_IOSTANDARD, "io-standard", NULL, true), > > +}; #endif > > + > > +static struct pinctrl_desc zynqmp_desc; > > + > > +/** > > + * zynqmp_pctrl_get_groups_count() - get group count > > + * @pctldev: Pincontrol device pointer. > > + * > > + * Get total groups count. > > + * > > + * Return: group count. > > + */ > > +static int zynqmp_pctrl_get_groups_count(struct pinctrl_dev *pctldev) > > +{ > > + struct zynqmp_pinctrl *pctrl = > > +pinctrl_dev_get_drvdata(pctldev); > > + > > + return pctrl->ngroups; > > +} > > + > > +/** > > + * zynqmp_pctrl_get_group_name() - get group name > > + * @pctldev: Pincontrol device pointer. > > + * @selector: Group ID. > > > + * > > + * Get gorup's name. > > + * > > + * Return: group name. > > Isn't too much duplication without any added value for the reader? I will remove the kernel doc for the obvious ones in the next series. > > > + */ > > +static const char *zynqmp_pctrl_get_group_name(struct pinctrl_dev > *pctldev, > > + unsigned int selector) > > +{ > > + struct zynqmp_pinctrl *pctrl = > > +pinctrl_dev_get_drvdata(pctldev); > > + > > + return pctrl->groups[selector].name; } > > + > > +/** > > + * zynqmp_pctrl_get_group_pins() - get group pins > > + * @pctldev: Pincontrol device pointer. > > + * @selector: Group ID. > > + * @pins: Pin numbers. > > + * @npins: Number of pins in group. > > + * > > + * Get gorup's pin count and pin number. > > > + * Return: Success. > > Clean up all your kernel docs from noise like this. I will remove the documentation for the obvious ones in the next series. > > > + */ > > +static int zynqmp_pctrl_get_group_pins(struct pinctrl_dev *pctldev, > > + unsigned int selector, > > + const unsigned int **pins, > > + unsigned int *npins) { > > + struct zynqmp_pinctrl *pctrl = > > +pinctrl_dev_get_drvdata(pctldev); > > + > > + *pins = pctrl->groups[selector].pins; > > + *npins = pctrl->groups[selector].npins; > > + > > + return 0; > > +} > > + > > +static const struct pinctrl_ops zynqmp_pctrl_ops = { > > + .get_groups_count = zynqmp_pctrl_get_groups_count, > > + .get_group_name = zynqmp_pctrl_get_group_name, > > + .get_group_pins = zynqmp_pctrl_get_group_pins, > > + .dt_node_to_map = pinconf_generic_dt_node_to_map_all, > > + .dt_free_map = pinctrl_utils_free_map, }; > > + > > +/** > > + * zynqmp_pinmux_request_pin() - Request a pin for muxing > > + * @pctldev: Pincontrol device pointer. > > + * @pin: Pin number. > > + * > > + * Request a pin from firmware for muxing. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinmux_request_pin(struct pinctrl_dev *pctldev, > > + unsigned int pin) { > > + int ret; > > + > > + ret = zynqmp_pm_pinctrl_request(pin); > > + if (ret) { > > + dev_err(pctldev->dev, "request failed for pin %u\n", > > + pin); > > > + return -EIO; > > Why shadowing error code? Since it's the only possible error, why is it not > reflected in the kernel doc? I will update the kernel doc with the error value for such cases. > > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * zynqmp_pmux_get_functions_count() - get number of functions > > + * @pctldev: Pincontrol device pointer. > > + * > > + * Get total function count. > > + * > > + * Return: function count. > > + */ > > +static int zynqmp_pmux_get_functions_count(struct pinctrl_dev > > +*pctldev) { > > + struct zynqmp_pinctrl *pctrl = > > +pinctrl_dev_get_drvdata(pctldev); > > + > > + return pctrl->nfuncs; > > +} > > + > > +/** > > + * zynqmp_pmux_get_function_name() - get function name > > + * @pctldev: Pincontrol device pointer. > > + * @selector: Function ID. > > + * > > + * Get function's name. > > + * > > + * Return: function name. > > + */ > > +static const char *zynqmp_pmux_get_function_name(struct pinctrl_dev > *pctldev, > > + unsigned int > > +selector) { > > + struct zynqmp_pinctrl *pctrl = > > +pinctrl_dev_get_drvdata(pctldev); > > + > > + return pctrl->funcs[selector].name; } > > + > > +/** > > + * zynqmp_pmux_get_function_groups() - Get groups for the function > > + * @pctldev: Pincontrol device pointer. > > + * @selector: Function ID > > + * @groups: Group names. > > + * @num_groups: Number of function groups. > > + * > > + * Get function's group count and group names. > > + * > > + * Return: Success. > > + */ > > +static int zynqmp_pmux_get_function_groups(struct pinctrl_dev *pctldev, > > + unsigned int selector, > > + const char * const **groups, > > + unsigned * const > > +num_groups) { > > + struct zynqmp_pinctrl *pctrl = > > +pinctrl_dev_get_drvdata(pctldev); > > + > > + *groups = pctrl->funcs[selector].groups; > > + *num_groups = pctrl->funcs[selector].ngroups; > > + > > + return 0; > > +} > > + > > +/** > > + * zynqmp_pinmux_set_mux() - Set requested function for the group > > + * @pctldev: Pincontrol device pointer. > > + * @function: Function ID. > > + * @group: Group ID. > > + * > > + * Loop though all pins of group and call firmware API > > + * to set requested function for all pins in group. > > through > of the group > in the group > > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinmux_set_mux(struct pinctrl_dev *pctldev, > > + unsigned int function, > > + unsigned int group) { > > + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > > + const struct zynqmp_pctrl_group *pgrp = &pctrl->groups[group]; > > + int ret, i; > > + > > + for (i = 0; i < pgrp->npins; i++) { > > + unsigned int pin = pgrp->pins[i]; > > + > > + ret = zynqmp_pm_pinctrl_set_function(pin, function); > > + if (ret) { > > + dev_err(pctldev->dev, "set mux failed for pin %u\n", > > + pin); > > > + return -EIO; > > Same as above. > And applies to all similar cases. > > > + } > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * zynqmp_pinmux_release_pin() - Release a pin > > + * @pctldev: Pincontrol device pointer. > > + * @pin: Pin number. > > + * > > + * Release a pin from firmware. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinmux_release_pin(struct pinctrl_dev *pctldev, > > + unsigned int pin) { > > + int ret; > > + > > + ret = zynqmp_pm_pinctrl_release(pin); > > + if (ret) { > > + dev_err(pctldev->dev, "free pin failed for pin %u\n", > > + pin); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > + > > +static const struct pinmux_ops zynqmp_pinmux_ops = { > > + .request = zynqmp_pinmux_request_pin, > > + .get_functions_count = zynqmp_pmux_get_functions_count, > > + .get_function_name = zynqmp_pmux_get_function_name, > > + .get_function_groups = zynqmp_pmux_get_function_groups, > > + .set_mux = zynqmp_pinmux_set_mux, > > + .free = zynqmp_pinmux_release_pin, }; > > + > > +/** > > + * zynqmp_pinconf_cfg_get() - get config value for the pin > > + * @pctldev: Pin control device pointer. > > + * @pin: Pin number. > > + * @config: Value of config param. > > + * > > + * Get value of the requested configuration parameter for the > > + * given pin. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinconf_cfg_get(struct pinctrl_dev *pctldev, > > + unsigned int pin, > > + unsigned long *config) { > > + int ret; > > + unsigned int arg = 0, param = > > +pinconf_to_config_param(*config); > > Reversed xmas tree order? > Assignment of arg AFAICS is redundant. I will update this in the next version. > > > + if (pin >= zynqmp_desc.npins) > > + return -EOPNOTSUPP; > > + > > + switch (param) { > > + case PIN_CONFIG_SLEW_RATE: > > + param = PM_PINCTRL_CONFIG_SLEW_RATE; > > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); > > + break; > > + case PIN_CONFIG_BIAS_PULL_UP: > > + param = PM_PINCTRL_CONFIG_PULL_CTRL; > > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); > > + if (arg != PM_PINCTRL_BIAS_PULL_UP) > > + return -EINVAL; > > + > > + arg = 1; > > + break; > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > + param = PM_PINCTRL_CONFIG_PULL_CTRL; > > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); > > + if (arg != PM_PINCTRL_BIAS_PULL_DOWN) > > + return -EINVAL; > > + > > + arg = 1; > > + break; > > + case PIN_CONFIG_BIAS_DISABLE: > > + param = PM_PINCTRL_CONFIG_BIAS_STATUS; > > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); > > + if (arg != PM_PINCTRL_BIAS_DISABLE) > > + return -EINVAL; > > + > > + arg = 1; > > + break; > > + case PIN_CONFIG_IOSTANDARD: > > + param = PM_PINCTRL_CONFIG_VOLTAGE_STATUS; > > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); > > + break; > > + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: > > + param = PM_PINCTRL_CONFIG_SCHMITT_CMOS; > > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); > > + break; > > + case PIN_CONFIG_DRIVE_STRENGTH: > > + param = PM_PINCTRL_CONFIG_DRIVE_STRENGTH; > > + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); > > + switch (arg) { > > + case PM_PINCTRL_DRIVE_STRENGTH_2MA: > > + arg = DRIVE_STRENGTH_2MA; > > + break; > > + case PM_PINCTRL_DRIVE_STRENGTH_4MA: > > + arg = DRIVE_STRENGTH_4MA; > > + break; > > + case PM_PINCTRL_DRIVE_STRENGTH_8MA: > > + arg = DRIVE_STRENGTH_8MA; > > + break; > > + case PM_PINCTRL_DRIVE_STRENGTH_12MA: > > + arg = DRIVE_STRENGTH_12MA; > > + break; > > + default: > > + /* Invalid drive strength */ > > + dev_warn(pctldev->dev, > > + "Invalid drive strength for pin %d\n", > > + pin); > > + return -EINVAL; > > + } > > + break; > > + default: > > + ret = -EOPNOTSUPP; > > + break; > > + } > > + > > + param = pinconf_to_config_param(*config); > > + *config = pinconf_to_config_packed(param, arg); > > + > > + return ret; > > This is wrong. You have to return the error codes directly and do not touch > *config as long as error happens. I will update the *config and param under if (!ret) condition. > > > +} > > + > > +/** > > + * zynqmp_pinconf_cfg_set() - Set requested config for the pin > > + * @pctldev: Pincontrol device pointer. > > + * @pin: Pin number. > > + * @configs: Configuration to set. > > + * @num_configs: Number of configurations. > > + * > > > + * Loop though all configurations and call firmware API > > through > > > + * to set requested configurations for the pin. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinconf_cfg_set(struct pinctrl_dev *pctldev, > > + unsigned int pin, unsigned long *configs, > > + unsigned int num_configs) { > > + int i, ret; > > + > > + if (pin >= zynqmp_desc.npins) > > + return -EOPNOTSUPP; > > + > > + for (i = 0; i < num_configs; i++) { > > + unsigned int param = pinconf_to_config_param(configs[i]); > > + unsigned int arg = pinconf_to_config_argument(configs[i]); > > + unsigned int value; > > + > > + switch (param) { > > + case PIN_CONFIG_SLEW_RATE: > > + param = PM_PINCTRL_CONFIG_SLEW_RATE; > > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); > > + break; > > + case PIN_CONFIG_BIAS_PULL_UP: > > + param = PM_PINCTRL_CONFIG_PULL_CTRL; > > + arg = PM_PINCTRL_BIAS_PULL_UP; > > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); > > + break; > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > + param = PM_PINCTRL_CONFIG_PULL_CTRL; > > + arg = PM_PINCTRL_BIAS_PULL_DOWN; > > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); > > + break; > > + case PIN_CONFIG_BIAS_DISABLE: > > + param = PM_PINCTRL_CONFIG_BIAS_STATUS; > > + arg = PM_PINCTRL_BIAS_DISABLE; > > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); > > + break; > > + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: > > + param = PM_PINCTRL_CONFIG_SCHMITT_CMOS; > > + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); > > + break; > > + case PIN_CONFIG_DRIVE_STRENGTH: > > + switch (arg) { > > + case DRIVE_STRENGTH_2MA: > > + value = PM_PINCTRL_DRIVE_STRENGTH_2MA; > > + break; > > + case DRIVE_STRENGTH_4MA: > > + value = PM_PINCTRL_DRIVE_STRENGTH_4MA; > > + break; > > + case DRIVE_STRENGTH_8MA: > > + value = PM_PINCTRL_DRIVE_STRENGTH_8MA; > > + break; > > + case DRIVE_STRENGTH_12MA: > > + value = PM_PINCTRL_DRIVE_STRENGTH_12MA; > > + break; > > + default: > > + /* Invalid drive strength */ > > + dev_warn(pctldev->dev, > > + "Invalid drive strength for pin %d\n", > > + pin); > > + return -EINVAL; > > + } > > + > > + param = PM_PINCTRL_CONFIG_DRIVE_STRENGTH; > > + ret = zynqmp_pm_pinctrl_set_config(pin, param, value); > > + break; > > + case PIN_CONFIG_IOSTANDARD: > > + param = PM_PINCTRL_CONFIG_VOLTAGE_STATUS; > > + ret = zynqmp_pm_pinctrl_get_config(pin, param, > > + &value); > > + > > + if (arg != value) > > + dev_warn(pctldev->dev, > > + "Invalid IO Standard requested for pin %d\n", > > + pin); > > + > > + break; > > + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE: > > + case PIN_CONFIG_LOW_POWER_MODE: > > + /* > > + * These cases are mentioned in dts but configurable > > + * registers are unknown. So falling through to ignore > > + * boot time warnings as of now. > > + */ > > + ret = 0; > > + break; > > + default: > > + dev_warn(pctldev->dev, > > + "unsupported configuration parameter '%u'\n", > > + param); > > + ret = -EOPNOTSUPP; > > + break; > > + } > > + > > + param = pinconf_to_config_param(configs[i]); > > + arg = pinconf_to_config_argument(configs[i]); > > + if (ret) > > + dev_warn(pctldev->dev, > > + "%s failed: pin %u param %u value %u\n", > > + __func__, pin, param, arg); > > + } > > Remove all these __func__. In a properly formed (unique) message base you > don't need it. For the debugging Dynamic debug and other mechanisms allow > to get it at run-time w/o code recompilation. I will remove using '__func__' in the next series. > > > + return 0; > > +} > > + > > +/** > > + * zynqmp_pinconf_group_set() - Set requested config for the group > > + * @pctldev: Pincontrol device pointer. > > + * @selector: Group ID. > > + * @configs: Configuration to set. > > + * @num_configs: Number of configurations. > > + * > > + * Call function to set configs for each pin in group. > > in the group I will update in next series. > > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinconf_group_set(struct pinctrl_dev *pctldev, > > + unsigned int selector, > > + unsigned long *configs, > > + unsigned int num_configs) { > > + int i, ret; > > + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); > > + const struct zynqmp_pctrl_group *pgrp = > > +&pctrl->groups[selector]; > > + > > + for (i = 0; i < pgrp->npins; i++) { > > + ret = zynqmp_pinconf_cfg_set(pctldev, pgrp->pins[i], configs, > > + num_configs); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static const struct pinconf_ops zynqmp_pinconf_ops = { > > + .is_generic = true, > > + .pin_config_get = zynqmp_pinconf_cfg_get, > > + .pin_config_set = zynqmp_pinconf_cfg_set, > > + .pin_config_group_set = zynqmp_pinconf_group_set, }; > > + > > +static struct pinctrl_desc zynqmp_desc = { > > + .name = "zynqmp_pinctrl", > > + .owner = THIS_MODULE, > > + .pctlops = &zynqmp_pctrl_ops, > > + .pmxops = &zynqmp_pinmux_ops, > > + .confops = &zynqmp_pinconf_ops, #ifdef CONFIG_DEBUG_FS > > + .custom_conf_items = zynqmp_conf_items, #endif }; > > + > > +/** > > + * zynqmp_pinctrl_get_function_groups() - get groups for the function > > + * @fid: Function ID. > > + * @index: Group index. > > + * @groups: Groups data. > > + * > > + * Call firmware API to get groups for the given function. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_get_function_groups(u32 fid, u32 index, u16 > > +*groups) { > > + struct zynqmp_pm_query_data qdata = {0}; > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > ret_ is confusing. Drop it. Ok, will just drop 'ret_' prefix and simply use paylod in next version. > > > + int ret; > > + > > + qdata.qid = PM_QID_PINCTRL_GET_FUNCTION_GROUPS; > > + qdata.arg1 = fid; > > + qdata.arg2 = index; > > + > > + ret = zynqmp_pm_query_data(qdata, ret_payload); > > + if (ret) > > + return ret; > > + > > + memcpy(groups, &ret_payload[1], > > + PINCTRL_GET_FUNC_GROUPS_RESP_LEN); > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_get_func_num_groups() - get number of groups in > function > > + * @fid: Function ID. > > + * @ngroups: Number of groups in function. > > + * > > + * Call firmware API to get number of group in function. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_get_func_num_groups(u32 fid, unsigned int > > +*ngroups) { > > + struct zynqmp_pm_query_data qdata = {0}; > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > + int ret; > > + > > + qdata.qid = PM_QID_PINCTRL_GET_NUM_FUNCTION_GROUPS; > > + qdata.arg1 = fid; > > + > > + ret = zynqmp_pm_query_data(qdata, ret_payload); > > + if (ret) > > + return ret; > > + > > + *ngroups = ret_payload[1]; > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_prepare_func_groups() - prepare function and groups > data > > + * @dev: Device pointer. > > + * @fid: Function ID. > > + * @func: Function data. > > + * @groups: Groups data. > > + * > > + * Query firmware to get group IDs for each function. Firmware > > +returns > > + * group IDs. Based on gorup index for the function, group names in > > group > > > + * function are stored. For example, first gorup in "eth0" function > > the function > the first > group > > > + * is named as "eth0_0", second as "eth0_1" and so on. > > Spell check your comments. Will check and fix in the next version. > > > + * > > + * Based on group ID received from firmware, function stores name of > > + * group for that group ID. For an example, if "eth0" first group ID > > + * is x, groups[x] name will be stored as "eth0_0". > > + * > > + * Once done for each function, each function would have its group > > +names, > > + * and each groups would also have their names. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_prepare_func_groups(struct device *dev, u32 > fid, > > + struct zynqmp_pmux_function *func, > > + struct > > +zynqmp_pctrl_group *groups) { > > + u16 resp[NUM_GROUPS_PER_RESP] = {0}; > > + const char **fgroups; > > + int ret = 0, index, i; > > > + fgroups = devm_kzalloc(dev, sizeof(*fgroups) * func->ngroups, > > + GFP_KERNEL); > > One line With single line it is crossing 80 line bar and getting the checkpatch warning, hence split into two lines. > > > + if (!fgroups) > > + return -ENOMEM; > > + > > + for (index = 0; index < func->ngroups; index += > NUM_GROUPS_PER_RESP) { > > + ret = zynqmp_pinctrl_get_function_groups(fid, index, resp); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < NUM_GROUPS_PER_RESP; i++) { > > + if (resp[i] == (u16)NA_GROUP) > > + goto done; > > + > > + if (resp[i] == (u16)RESERVED_GROUP) > > + continue; > > + > > + fgroups[index + i] = devm_kasprintf(dev, GFP_KERNEL, > > + "%s_%d_grp", > > + func->name, > > + index + i); > > + groups[resp[i]].name = devm_kasprintf(dev, GFP_KERNEL, > > + "%s_%d_grp", > > + func->name, > > + index + > > + i); > > No NULL checks?! I will update in the next series. > > > + } > > + } > > +done: > > + func->groups = fgroups; > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_get_function_name() - get function name > > + * @fid: Function ID. > > + * @name: Function name > > + * > > + * Call firmware API to get name of given function. > > + */ > > +static void zynqmp_pinctrl_get_function_name(u32 fid, char *name) { > > + struct zynqmp_pm_query_data qdata = {0}; > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > + > > + qdata.qid = PM_QID_PINCTRL_GET_FUNCTION_NAME; > > + qdata.arg1 = fid; > > + > > + /* > > + * Name of the function is maximum 16 bytes and cannot > > + * accommodate the return value in SMC buffers, hence ignoring > > + * the return value for this specific qid. > > + */ > > + zynqmp_pm_query_data(qdata, ret_payload); > > + memcpy(name, ret_payload, PINCTRL_GET_FUNC_NAME_RESP_LEN); > } > > + > > +/** > > + * zynqmp_pinctrl_get_num_functions() - get number of supported > functions > > + * @nfuncs: Number of functions. > > + * > > + * Call firmware API to get number of functions supported by > system/board. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_get_num_functions(unsigned int *nfuncs) { > > > + struct zynqmp_pm_query_data qdata = {0}; > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > Drop in all cases those redundant prefixes. > > > + int ret; > > + > > + qdata.qid = PM_QID_PINCTRL_GET_NUM_FUNCTIONS; > > + > > + ret = zynqmp_pm_query_data(qdata, ret_payload); > > + if (ret) > > + return ret; > > + > > + *nfuncs = ret_payload[1]; > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_get_pin_groups() - get groups for the pin > > + * @pin: Pin number. > > + * @index: Group index. > > + * @groups: Groups data. > > + * > > + * Call firmware API to get groups for the given pin. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_get_pin_groups(u32 pin, u32 index, u16 > > +*groups) { > > + struct zynqmp_pm_query_data qdata = {0}; > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > + int ret; > > + > > + qdata.qid = PM_QID_PINCTRL_GET_PIN_GROUPS; > > + qdata.arg1 = pin; > > + qdata.arg2 = index; > > + > > + ret = zynqmp_pm_query_data(qdata, ret_payload); > > + if (ret) > > + return ret; > > + > > + memcpy(groups, &ret_payload[1], > > + PINCTRL_GET_PIN_GROUPS_RESP_LEN); > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_group_add_pin() - add pin to given group > > + * @group: Group data. > > + * @pin: Pin number. > > + * > > + * Add pin number to respective group's pin array at end and > > + * increment pin count for the group. > > + */ > > +static void zynqmp_pinctrl_group_add_pin(struct zynqmp_pctrl_group > *group, > > + unsigned int pin) { > > + group->pins[group->npins++] = pin; } > > + > > +/** > > + * zynqmp_pinctrl_create_pin_groups() - assign pins to respective groups > > + * @dev: Device pointer. > > + * @groups: Groups data. > > + * @pin: Pin number. > > + * > > + * Query firmware to get groups available for the given pin. > > + * Based on firmware response(group IDs for the pin), add > > + * pin number to respective group's pin array. > > + * > > + * Once all pins are queries, each groups would have its number > > + * of pins and pin numbers data. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_create_pin_groups(struct device *dev, > > + struct zynqmp_pctrl_group *groups, > > + unsigned int pin) { > > + int ret, i, index = 0; > > + u16 resp[NUM_GROUPS_PER_RESP] = {0}; > > + > > + do { > > + ret = zynqmp_pinctrl_get_pin_groups(pin, index, resp); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < NUM_GROUPS_PER_RESP; i++) { > > + if (resp[i] == (u16)NA_GROUP) > > + return ret; > > + > > + if (resp[i] == (u16)RESERVED_GROUP) > > + continue; > > In the entire driver remove all explicit castings where it's not needed, like > above. If something is not okay with it, fix the root cause. Ok, I will check this and fix in next version. > > > + zynqmp_pinctrl_group_add_pin(&groups[resp[i]], pin); > > + } > > + index += NUM_GROUPS_PER_RESP; > > + } while (1); > > Try to refactor to avoid infinite loops. I will check and update this in next version. > > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_prepare_group_pins() - prepare each group's pin data > > + * @dev: Device pointer. > > + * @groups: Groups data. > > + * @ngroups: Number of groups. > > + * > > + * Prepare pin number and number of pins data for each pins. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_prepare_group_pins(struct device *dev, > > + struct zynqmp_pctrl_group *groups, > > + unsigned int ngroups) { > > + unsigned int pin; > > + int ret = 0; > > Redundant assignment. Static analyzer tool will throw warning as it expects the variable to be Initialized in all possible paths. I will cross check on this and remove if it is not the case. > > > + for (pin = 0; pin < zynqmp_desc.npins; pin++) { > > + ret = zynqmp_pinctrl_create_pin_groups(dev, groups, pin); > > + if (ret) > > + return ret; > > + } > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_prepare_function_info() - prepare function info > > + * @dev: Device pointer. > > + * @pctrl: Pin control driver data. > > + * > > + * Query firmware for functions, groups and pin information and > > + * prepare pin control driver data. > > + * > > + * Query number of functions and number of function groups (number > > + * of groups in given function) to allocate required memory buffers > > + * for functions and groups. Once buffers are allocated to store > > + * functions and groups data, query and store required information > > + * (numbe of groups and group names for each function, number of > > number > > > + * pins and pin numbers for each group). > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_prepare_function_info(struct device *dev, > > + struct zynqmp_pinctrl > > +*pctrl) { > > + struct zynqmp_pmux_function *funcs; > > + struct zynqmp_pctrl_group *groups; > > + int ret, i; > > + > > + ret = zynqmp_pinctrl_get_num_functions(&pctrl->nfuncs); > > + if (ret) > > + return ret; > > + > > + funcs = devm_kzalloc(dev, sizeof(*funcs) * pctrl->nfuncs, > GFP_KERNEL); > > + if (!funcs) > > + return -ENOMEM; > > + > > + for (i = 0; i < pctrl->nfuncs; i++) { > > + zynqmp_pinctrl_get_function_name(i, funcs[i].name); > > + > > + ret = zynqmp_pinctrl_get_func_num_groups(i, > &funcs[i].ngroups); > > + if (ret) > > + return ret; > > + > > + pctrl->ngroups += funcs[i].ngroups; > > + } > > > + groups = devm_kzalloc(dev, sizeof(*groups) * pctrl->ngroups, > > + GFP_KERNEL); > > One line. It will cross 80 line mark if we make it to a single line. > > > + if (!groups) > > + return -ENOMEM; > > + > > + for (i = 0; i < pctrl->nfuncs; i++) { > > + ret = zynqmp_pinctrl_prepare_func_groups(dev, i, &funcs[i], > > + groups); > > + if (ret) > > + return ret; > > + } > > + > > + ret = zynqmp_pinctrl_prepare_group_pins(dev, groups, pctrl- > >ngroups); > > + if (ret) > > + return ret; > > + > > + pctrl->funcs = funcs; > > + pctrl->groups = groups; > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_get_num_pins() - get number of pins in system > > + * @npins: Number of pins in system/board. > > + * > > + * Call firmware API to get number of pins. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_get_num_pins(unsigned int *npins) { > > + struct zynqmp_pm_query_data qdata = {0}; > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > + int ret; > > + > > + qdata.qid = PM_QID_PINCTRL_GET_NUM_PINS; > > + > > + ret = zynqmp_pm_query_data(qdata, ret_payload); > > + if (ret) > > + return ret; > > + > > + *npins = ret_payload[1]; > > + > > + return ret; > > +} > > + > > +/** > > + * zynqmp_pinctrl_prepare_pin_desc() - prepare pin description info > > + * @dev: Device pointer. > > + * @zynqmp_pins: Pin information. > > + * @npins: Number of pins. > > + * > > + * Query number of pins information from firmware and prepare pin > > + * description containing pin number and pin name. > > + * > > + * Return: 0 on success else error code. > > + */ > > +static int zynqmp_pinctrl_prepare_pin_desc(struct device *dev, > > + const struct pinctrl_pin_desc > > + **zynqmp_pins, > > + unsigned int *npins) { > > + struct pinctrl_pin_desc *pins, *pin; > > + int ret; > > + int i; > > + > > + ret = zynqmp_pinctrl_get_num_pins(npins); > > + if (ret) > > + return ret; > > + > > + pins = devm_kzalloc(dev, sizeof(*pins) * *npins, GFP_KERNEL); > > + if (!pins) > > + return -ENOMEM; > > + > > + for (i = 0; i < *npins; i++) { > > + pin = &pins[i]; > > + pin->number = i; > > + pin->name = devm_kasprintf(dev, GFP_KERNEL, "%s%d", > > + ZYNQMP_PIN_PREFIX, i); > > no NULL check?! I will update in the next series. > > + } > > + > > + *zynqmp_pins = pins; > > + > > + return 0; > > +} > > + > > +static int zynqmp_pinctrl_probe(struct platform_device *pdev) { > > + struct zynqmp_pinctrl *pctrl; > > + int ret; > > + > > + pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL); > > + if (!pctrl) > > + return -ENOMEM; > > + > > + ret = zynqmp_pinctrl_prepare_pin_desc(&pdev->dev, > > + &zynqmp_desc.pins, > > + &zynqmp_desc.npins); > > + if (ret) { > > + dev_err(&pdev->dev, "%s() pin desc prepare fail with %d\n", > > + __func__, ret); > > + return ret; > > + } > > + > > + ret = zynqmp_pinctrl_prepare_function_info(&pdev->dev, pctrl); > > + if (ret) { > > + dev_err(&pdev->dev, "%s() function info prepare fail with %d\n", > > + __func__, ret); > > + return ret; > > + } > > + > > + pctrl->pctrl = pinctrl_register(&zynqmp_desc, &pdev->dev, pctrl); > > + if (IS_ERR(pctrl->pctrl)) { > > > + ret = PTR_ERR(pctrl->pctrl); > > + return ret; > > Fold it and drop {}. I will change this to 'return PTR_ERR(pctrl->pctrl);' in next version. > > > + } > > + > > + platform_set_drvdata(pdev, pctrl); > > > + dev_info(&pdev->dev, "zynqmp pinctrl initialized\n"); > > Unneeded noise. I will remove this in next version. > > > + return ret; > > +} > > + > > +static const struct of_device_id zynqmp_pinctrl_of_match[] = { > > + { .compatible = "xlnx,zynqmp-pinctrl" }, > > + { } > > +}; > > + > > +static struct platform_driver zynqmp_pinctrl_driver = { > > + .driver = { > > + .name = "zynqmp-pinctrl", > > + .of_match_table = zynqmp_pinctrl_of_match, > > + }, > > + .probe = zynqmp_pinctrl_probe, }; > > +builtin_platform_driver(zynqmp_pinctrl_driver); > > I believe the code base can be easily shrinked by 10%. So, next version I > would expect something like < 1000 LOCs in the stat. By removing the kernel docs for the obvious ones will definitely reach. Regards Sai Krishna > > -- > With Best Regards, > Andy Shevchenko
On Thu, Mar 18, 2021 at 4:42 PM Sai Krishna Potthuri <lakshmis@xilinx.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Wednesday, March 17, 2021 6:26 PM > > On Wed, Mar 17, 2021 at 10:27 AM Sai Krishna Potthuri > > <lakshmi.sai.krishna.potthuri@xilinx.com> wrote: ... > > > +config PINCTRL_ZYNQMP > > > + bool "Pinctrl driver for Xilinx ZynqMP" > > > > Why not module? > As most of the Xilinx drivers depending on the pin controller driver for > configuring the MIO pins, we are not supporting to build this driver as > a module. OK. > > > + depends on ARCH_ZYNQMP > > > + select PINMUX > > > + select GENERIC_PINCONF ... > > > +#include <linux/init.h> > > > +#include <linux/of_address.h> > > > > > +#include <linux/pinctrl/pinmux.h> > > > +#include <linux/pinctrl/pinconf-generic.h> > > > > Can you move this group of headers after linux/* ? > > > > > +#include <dt-bindings/pinctrl/pinctrl-zynqmp.h> > > > > Can it be moved outside of these headers > > > > > +#include <linux/platform_device.h> > > > +#include <linux/firmware/xlnx-zynqmp.h> > > > > Ordering (for all groups of headers)? > Ok, I will order the headers in the below order > #include <linux/*> > #include <linux/firmware/xlnx-zynqmp.h> + blank line > #include <linux/pinctrl/*> + blank line > #include <dt-bindings/pinctrl/pinctrl-zynqmp.h> Looking into other drivers with similar includes, shouldn't it go first in the file before any other linux/* asm/* etc? > > > +#include "core.h" > > > +#include "pinctrl-utils.h" ... > > > + PIN_CONFIG_IOSTANDARD = PIN_CONFIG_END + 1, }; > > > > I'm lost here. What is IO standard exactly? Why it can't be in generic > > headers? > It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts. > Since this is specific to Xilinx ZynqMP platform, considered to be added in > the driver file. So, why can't we create a couple of bits to represent this voltages in the generic header and gain usability for others as well? Linus? ... > > > + ret = zynqmp_pm_pinctrl_request(pin); > > > + if (ret) { > > > + dev_err(pctldev->dev, "request failed for pin %u\n", > > > + pin); > > > > > + return -EIO; > > > > Why shadowing error code? So, any comments on the initial Q? >> Since it's the only possible error, why is it not > > reflected in the kernel doc? > I will update the kernel doc with the error value for such cases. > > > > > + } ... > > > + default: > > > + /* Invalid drive strength */ > > > + dev_warn(pctldev->dev, > > > + "Invalid drive strength for pin %d\n", > > > + pin); > > > + return -EINVAL; > > > + } > > > + break; > > > + default: > > > + ret = -EOPNOTSUPP; > > > + break; > > > + } > > > + > > > + param = pinconf_to_config_param(*config); > > > + *config = pinconf_to_config_packed(param, arg); > > > + > > > + return ret; > > > > This is wrong. You have to return the error codes directly and do not touch > > *config as long as error happens. > I will update the *config and param under if (!ret) condition. Simpler and better just to return errors immediately from the switch-case entries. ... > > > + fgroups = devm_kzalloc(dev, sizeof(*fgroups) * func->ngroups, > > > + GFP_KERNEL); > > > > One line > With single line it is crossing 80 line bar and getting the checkpatch warning, > hence split into two lines. No, you may not get a checkpatch warning. Are you working on v5.4 kernels or earlier? > > > + if (!fgroups) > > > + return -ENOMEM; ... > > > +static int zynqmp_pinctrl_prepare_group_pins(struct device *dev, > > > + struct zynqmp_pctrl_group *groups, > > > + unsigned int ngroups) { > > > + unsigned int pin; > > > + int ret = 0; > > > > Redundant assignment. > Static analyzer tool will throw warning as it expects the variable to be > Initialized in all possible paths. Because you need to explicitly return 0 at the end of the function. Don't follow static analyzers or other tools blindly. Think about all aspects. > I will cross check on this and remove if it is not the case. > > > > > + for (pin = 0; pin < zynqmp_desc.npins; pin++) { > > > + ret = zynqmp_pinctrl_create_pin_groups(dev, groups, pin); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return ret; > > > +} ... > > > + groups = devm_kzalloc(dev, sizeof(*groups) * pctrl->ngroups, > > > + GFP_KERNEL); > > > > One line. > It will cross 80 line mark if we make it to a single line. I don't think it's a problem in this case. > > > + if (!groups) > > > + return -ENOMEM; -- With Best Regards, Andy Shevchenko
Hi Andy Shevchenko, > -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Friday, March 19, 2021 3:53 PM > To: Sai Krishna Potthuri <lakshmis@xilinx.com> > Cc: Linus Walleij <linus.walleij@linaro.org>; Rob Herring > <robh+dt@kernel.org>; Michal Simek <michals@xilinx.com>; Greg Kroah- > Hartman <gregkh@linuxfoundation.org>; linux-arm Mailing List <linux-arm- > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; devicetree <devicetree@vger.kernel.org>; open > list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>; git <git@xilinx.com>; > saikrishna12468@gmail.com > Subject: Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support > > On Thu, Mar 18, 2021 at 4:42 PM Sai Krishna Potthuri <lakshmis@xilinx.com> > wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Sent: Wednesday, March 17, 2021 6:26 PM On Wed, Mar 17, 2021 at > > > 10:27 AM Sai Krishna Potthuri > > > <lakshmi.sai.krishna.potthuri@xilinx.com> wrote: > > ... > > > > > +config PINCTRL_ZYNQMP > > > > + bool "Pinctrl driver for Xilinx ZynqMP" > > > > > > Why not module? > > As most of the Xilinx drivers depending on the pin controller driver > > for configuring the MIO pins, we are not supporting to build this > > driver as a module. > > OK. > > > > > + depends on ARCH_ZYNQMP > > > > + select PINMUX > > > > + select GENERIC_PINCONF > > ... > > > > > +#include <linux/init.h> > > > > +#include <linux/of_address.h> > > > > > > > +#include <linux/pinctrl/pinmux.h> #include > > > > +<linux/pinctrl/pinconf-generic.h> > > > > > > Can you move this group of headers after linux/* ? > > > > > > > +#include <dt-bindings/pinctrl/pinctrl-zynqmp.h> > > > > > > Can it be moved outside of these headers > > > > > > > +#include <linux/platform_device.h> #include > > > > +<linux/firmware/xlnx-zynqmp.h> > > > > > > Ordering (for all groups of headers)? > > Ok, I will order the headers in the below order #include <linux/*> > > #include <linux/firmware/xlnx-zynqmp.h> > > + blank line > > > #include <linux/pinctrl/*> > > + blank line Ok, I will add above two blank lines. > > > #include <dt-bindings/pinctrl/pinctrl-zynqmp.h> > > Looking into other drivers with similar includes, shouldn't it go first in the file > before any other linux/* asm/* etc? I see some drivers are including this header before linux/* and some are using after linux/*. > > > > > +#include "core.h" > > > > +#include "pinctrl-utils.h" > > ... > > > > > + PIN_CONFIG_IOSTANDARD = PIN_CONFIG_END + 1, }; > > > > > > I'm lost here. What is IO standard exactly? Why it can't be in > > > generic headers? > > It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts. > > Since this is specific to Xilinx ZynqMP platform, considered to be > > added in the driver file. > > So, why can't we create a couple of bits to represent this voltages in the > generic header and gain usability for others as well? I see some drivers are maintaining the configuration list in the driver file, if the configuration is specific to the driver. We can move this to generic header if it is used by others as well. Ok, will wait for Linus to comment. > > Linus? > > ... > > > > > + ret = zynqmp_pm_pinctrl_request(pin); > > > > + if (ret) { > > > > + dev_err(pctldev->dev, "request failed for pin > > > > + %u\n", pin); > > > > > > > + return -EIO; > > > > > > Why shadowing error code? > > So, any comments on the initial Q? Xilinx low level secure firmware error codes are different from Linux error codes. Secure firmware maintains list of error codes (positive values other than zero). Hence we return -EIO, if the return value from firmware is non-zero. > > >> Since it's the only possible error, why is it not > > > reflected in the kernel doc? > > I will update the kernel doc with the error value for such cases. > > > > > > > + } > > ... > > > > > + default: > > > > + /* Invalid drive strength */ > > > > + dev_warn(pctldev->dev, > > > > + "Invalid drive strength for pin %d\n", > > > > + pin); > > > > + return -EINVAL; > > > > + } > > > > + break; > > > > + default: > > > > + ret = -EOPNOTSUPP; > > > > + break; > > > > + } > > > > + > > > > + param = pinconf_to_config_param(*config); > > > > + *config = pinconf_to_config_packed(param, arg); > > > > + > > > > + return ret; > > > > > > This is wrong. You have to return the error codes directly and do > > > not touch *config as long as error happens. > > I will update the *config and param under if (!ret) condition. > > Simpler and better just to return errors immediately from the switch-case > entries. Ok, I will update. > > ... > > > > > + fgroups = devm_kzalloc(dev, sizeof(*fgroups) * func->ngroups, > > > > + GFP_KERNEL); > > > > > > One line > > With single line it is crossing 80 line bar and getting the checkpatch > > warning, hence split into two lines. > > No, you may not get a checkpatch warning. Are you working on v5.4 kernels > or earlier? Ok, got it. Driver is developed on top of the older kernel (<=5.4) and we see this warning at that time and fixed by splitting into two lines. Yes, with the latest kernel I am not seeing the warning by keeping it in one line, I will update this. > > > > > + if (!fgroups) > > > > + return -ENOMEM; > > ... > > > > > +static int zynqmp_pinctrl_prepare_group_pins(struct device *dev, > > > > + struct zynqmp_pctrl_group *groups, > > > > + unsigned int ngroups) { > > > > + unsigned int pin; > > > > + int ret = 0; > > > > > > Redundant assignment. > > Static analyzer tool will throw warning as it expects the variable to > > be Initialized in all possible paths. > > Because you need to explicitly return 0 at the end of the function. > Don't follow static analyzers or other tools blindly. Think about all aspects. Ok, I will update this. > > > I will cross check on this and remove if it is not the case. > > > > > > > + for (pin = 0; pin < zynqmp_desc.npins; pin++) { > > > > + ret = zynqmp_pinctrl_create_pin_groups(dev, groups, pin); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + return ret; > > > > +} > > ... > > > > > + groups = devm_kzalloc(dev, sizeof(*groups) * pctrl->ngroups, > > > > + GFP_KERNEL); > > > > > > One line. > > It will cross 80 line mark if we make it to a single line. > > I don't think it's a problem in this case. I will update this. > > > > > + if (!groups) > > > > + return -ENOMEM; > > -- > With Best Regards, > Andy Shevchenko
On Mon, Mar 22, 2021 at 5:25 PM Sai Krishna Potthuri <lakshmis@xilinx.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Friday, March 19, 2021 3:53 PM > > On Thu, Mar 18, 2021 at 4:42 PM Sai Krishna Potthuri <lakshmis@xilinx.com> > > wrote: > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > Sent: Wednesday, March 17, 2021 6:26 PM On Wed, Mar 17, 2021 at > > > > 10:27 AM Sai Krishna Potthuri > > > > <lakshmi.sai.krishna.potthuri@xilinx.com> wrote: ... > > > #include <dt-bindings/pinctrl/pinctrl-zynqmp.h> > > > > Looking into other drivers with similar includes, shouldn't it go first in the file > > before any other linux/* asm/* etc? > I see some drivers are including this header before linux/* and some are using > after linux/*. The rule of thumb is that: more generic headers are going first. I consider dt/* ones are more generic than linux/* ones because they are covering more than just the Linux kernel. ... > > > > I'm lost here. What is IO standard exactly? Why it can't be in > > > > generic headers? > > > It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts. > > > Since this is specific to Xilinx ZynqMP platform, considered to be > > > added in the driver file. > > > > So, why can't we create a couple of bits to represent this voltages in the > > generic header and gain usability for others as well? > I see some drivers are maintaining the configuration list in the driver file, if > the configuration is specific to the driver. Yes, my point is that this case doesn't sound too specific to the driver. Many pin control buffers (in hardware way of speaking) have properties to be different voltage tolerant / produce. > We can move this to generic header if it is used by others as well. > Ok, will wait for Linus to comment. > > > > Linus? ... > > > > > + ret = zynqmp_pm_pinctrl_request(pin); > > > > > + if (ret) { > > > > > + dev_err(pctldev->dev, "request failed for pin > > > > > + %u\n", pin); > > > > > > > > > + return -EIO; > > > > > > > > Why shadowing error code? > > > > So, any comments on the initial Q? > Xilinx low level secure firmware error codes are different from Linux error codes. > Secure firmware maintains list of error codes (positive values other than zero). > Hence we return -EIO, if the return value from firmware is non-zero. Why the zynqmp_pm_pinctrl_request() can't return codes in Linux error code space? > > >> Since it's the only possible error, why is it not > > > > reflected in the kernel doc? > > > I will update the kernel doc with the error value for such cases. > > > > > > > > > + } -- With Best Regards, Andy Shevchenko
Hi Andy Shevchenko, > -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Monday, March 22, 2021 10:47 PM > To: Sai Krishna Potthuri <lakshmis@xilinx.com> > Cc: Linus Walleij <linus.walleij@linaro.org>; Rob Herring > <robh+dt@kernel.org>; Michal Simek <michals@xilinx.com>; Greg Kroah- > Hartman <gregkh@linuxfoundation.org>; linux-arm Mailing List <linux-arm- > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; devicetree <devicetree@vger.kernel.org>; open > list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>; git <git@xilinx.com>; > saikrishna12468@gmail.com > Subject: Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support > > On Mon, Mar 22, 2021 at 5:25 PM Sai Krishna Potthuri > <lakshmis@xilinx.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Sent: Friday, March 19, 2021 3:53 PM On Thu, Mar 18, 2021 at 4:42 PM > > > Sai Krishna Potthuri <lakshmis@xilinx.com> > > > wrote: > > > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > > Sent: Wednesday, March 17, 2021 6:26 PM On Wed, Mar 17, 2021 at > > > > > 10:27 AM Sai Krishna Potthuri > > > > > <lakshmi.sai.krishna.potthuri@xilinx.com> wrote: > > ... > > > > > #include <dt-bindings/pinctrl/pinctrl-zynqmp.h> > > > > > > Looking into other drivers with similar includes, shouldn't it go > > > first in the file before any other linux/* asm/* etc? > > I see some drivers are including this header before linux/* and some > > are using after linux/*. > > The rule of thumb is that: more generic headers are going first. > > I consider dt/* ones are more generic than linux/* ones because they are > covering more than just the Linux kernel. Ok, I will reorder accordingly. > > ... > > > > > > I'm lost here. What is IO standard exactly? Why it can't be in > > > > > generic headers? > > > > It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts. > > > > Since this is specific to Xilinx ZynqMP platform, considered to be > > > > added in the driver file. > > > > > > So, why can't we create a couple of bits to represent this voltages > > > in the generic header and gain usability for others as well? > > I see some drivers are maintaining the configuration list in the > > driver file, if the configuration is specific to the driver. > > Yes, my point is that this case doesn't sound too specific to the driver. Many > pin control buffers (in hardware way of speaking) have properties to be > different voltage tolerant / produce. Ok, you want me to proceed with this change or shall we wait for Linus to comment? > > > We can move this to generic header if it is used by others as well. > > Ok, will wait for Linus to comment. > > > > > > Linus? > > ... > > > > > > > + ret = zynqmp_pm_pinctrl_request(pin); > > > > > > + if (ret) { > > > > > > + dev_err(pctldev->dev, "request failed for pin > > > > > > + %u\n", pin); > > > > > > > > > > > + return -EIO; > > > > > > > > > > Why shadowing error code? > > > > > > So, any comments on the initial Q? > > Xilinx low level secure firmware error codes are different from Linux error > codes. > > Secure firmware maintains list of error codes (positive values other than > zero). > > Hence we return -EIO, if the return value from firmware is non-zero. > > Why the zynqmp_pm_pinctrl_request() can't return codes in Linux error > code space? I cross checked with the Xilinx firmware team, firmware driver is now returning Linux error codes in the latest kernel but while developing the driver it was a different approach. I will update the driver to simply return the values from firmware calls. > > > > >> Since it's the only possible error, why is it not > > > > > reflected in the kernel doc? > > > > I will update the kernel doc with the error value for such cases. > > > > > > > > > > > + } > > -- > With Best Regards, > Andy Shevchenko
On Wed, Mar 17, 2021 at 01:55:16PM +0530, Sai Krishna Potthuri wrote: > Adding pinctrl driver for Xilinx ZynqMP platform. > This driver queries pin information from firmware and registers > pin control accordingly. > > Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> > --- > drivers/pinctrl/Kconfig | 13 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/pinctrl-zynqmp.c | 1030 ++++++++++++++++++++++++++++++ > 3 files changed, 1044 insertions(+) > create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 815095326e2d..25d3c7208975 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ > help > This selects the pinctrl driver for Xilinx Zynq. > > +config PINCTRL_ZYNQMP > + bool "Pinctrl driver for Xilinx ZynqMP" Please make this work as a module. > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -43,6 +43,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o > obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o > obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o > obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o > +obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o Please use a tab like the other lines in this file. > + > + dev_info(&pdev->dev, "zynqmp pinctrl initialized\n"); When drivers work properly, they are quiet. Please remove this. thanks, greg k-h
On 3/23/21 2:42 PM, Greg Kroah-Hartman wrote: > On Wed, Mar 17, 2021 at 01:55:16PM +0530, Sai Krishna Potthuri wrote: >> Adding pinctrl driver for Xilinx ZynqMP platform. >> This driver queries pin information from firmware and registers >> pin control accordingly. >> >> Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> >> --- >> drivers/pinctrl/Kconfig | 13 + >> drivers/pinctrl/Makefile | 1 + >> drivers/pinctrl/pinctrl-zynqmp.c | 1030 ++++++++++++++++++++++++++++++ >> 3 files changed, 1044 insertions(+) >> create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c >> >> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig >> index 815095326e2d..25d3c7208975 100644 >> --- a/drivers/pinctrl/Kconfig >> +++ b/drivers/pinctrl/Kconfig >> @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ >> help >> This selects the pinctrl driver for Xilinx Zynq. >> >> +config PINCTRL_ZYNQMP >> + bool "Pinctrl driver for Xilinx ZynqMP" > > Please make this work as a module. The most of pinctrl drivers are builtin modules now which is not excuse it is just fact. $ git grep module_pla drivers/pinctrl/ | wc -l 40 $ git grep builtin_pla drivers/pinctrl/ | wc -l 64 Also at least last 3 pinctrl drivers which have been merged are not modules. d4c34d09ab03 ("pinctrl: Add RISC-V Canaan Kendryte K210 FPIOA driver") 7e5ea974e61c ("pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO") a68a7844264e ("pinctrl: visconti: Add Toshiba Visconti SoCs pinctrl support") None is saying that it can't be done but that cases where you use pinctrl as module are really very limited. When you start to use pinctrl and its functionality you need to have it as the part of the kernel to be to get console, mmc, ethernet, usb, etc. That's why I would like to know what functionality and use case you have in mind that this driver should be made module. Thanks, Michal
On Wed, Mar 24, 2021 at 09:29:12AM +0100, Michal Simek wrote: > On 3/23/21 2:42 PM, Greg Kroah-Hartman wrote: > > On Wed, Mar 17, 2021 at 01:55:16PM +0530, Sai Krishna Potthuri wrote: > >> Adding pinctrl driver for Xilinx ZynqMP platform. > >> This driver queries pin information from firmware and registers > >> pin control accordingly. > >> > >> Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> > >> --- > >> drivers/pinctrl/Kconfig | 13 + > >> drivers/pinctrl/Makefile | 1 + > >> drivers/pinctrl/pinctrl-zynqmp.c | 1030 ++++++++++++++++++++++++++++++ > >> 3 files changed, 1044 insertions(+) > >> create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c > >> > >> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > >> index 815095326e2d..25d3c7208975 100644 > >> --- a/drivers/pinctrl/Kconfig > >> +++ b/drivers/pinctrl/Kconfig > >> @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ > >> help > >> This selects the pinctrl driver for Xilinx Zynq. > >> > >> +config PINCTRL_ZYNQMP > >> + bool "Pinctrl driver for Xilinx ZynqMP" > > > > Please make this work as a module. > > The most of pinctrl drivers are builtin modules now which is not excuse > it is just fact. > $ git grep module_pla drivers/pinctrl/ | wc -l > 40 > $ git grep builtin_pla drivers/pinctrl/ | wc -l > 64 For new ones, we can do better, don't make us have to go back and fix this up later. > Also at least last 3 pinctrl drivers which have been merged are not modules. > d4c34d09ab03 ("pinctrl: Add RISC-V Canaan Kendryte K210 FPIOA driver") > 7e5ea974e61c ("pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for > Microsemi Serial GPIO") > a68a7844264e ("pinctrl: visconti: Add Toshiba Visconti SoCs pinctrl > support") > > None is saying that it can't be done but that cases where you use > pinctrl as module are really very limited. When you start to use pinctrl > and its functionality you need to have it as the part of the kernel to > be to get console, mmc, ethernet, usb, etc. > > That's why I would like to know what functionality and use case you have > in mind that this driver should be made module. The "functionality" of building a kernel image that works on all hardware types. Just like x86-64 has been for a very long time :) thanks, greg k-h
On 3/24/21 9:49 AM, Greg Kroah-Hartman wrote: > On Wed, Mar 24, 2021 at 09:29:12AM +0100, Michal Simek wrote: >> On 3/23/21 2:42 PM, Greg Kroah-Hartman wrote: >>> On Wed, Mar 17, 2021 at 01:55:16PM +0530, Sai Krishna Potthuri wrote: >>>> Adding pinctrl driver for Xilinx ZynqMP platform. >>>> This driver queries pin information from firmware and registers >>>> pin control accordingly. >>>> >>>> Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> >>>> --- >>>> drivers/pinctrl/Kconfig | 13 + >>>> drivers/pinctrl/Makefile | 1 + >>>> drivers/pinctrl/pinctrl-zynqmp.c | 1030 ++++++++++++++++++++++++++++++ >>>> 3 files changed, 1044 insertions(+) >>>> create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c >>>> >>>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig >>>> index 815095326e2d..25d3c7208975 100644 >>>> --- a/drivers/pinctrl/Kconfig >>>> +++ b/drivers/pinctrl/Kconfig >>>> @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ >>>> help >>>> This selects the pinctrl driver for Xilinx Zynq. >>>> >>>> +config PINCTRL_ZYNQMP >>>> + bool "Pinctrl driver for Xilinx ZynqMP" >>> >>> Please make this work as a module. >> >> The most of pinctrl drivers are builtin modules now which is not excuse >> it is just fact. >> $ git grep module_pla drivers/pinctrl/ | wc -l >> 40 >> $ git grep builtin_pla drivers/pinctrl/ | wc -l >> 64 > > For new ones, we can do better, don't make us have to go back and fix > this up later. As I said not a big deal. If this is the way to go then I these rules should be followed which is not what it is happening based on 3 latest pinctrl drivers below. > >> Also at least last 3 pinctrl drivers which have been merged are not modules. >> d4c34d09ab03 ("pinctrl: Add RISC-V Canaan Kendryte K210 FPIOA driver") >> 7e5ea974e61c ("pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for >> Microsemi Serial GPIO") >> a68a7844264e ("pinctrl: visconti: Add Toshiba Visconti SoCs pinctrl >> support") >> >> None is saying that it can't be done but that cases where you use >> pinctrl as module are really very limited. When you start to use pinctrl >> and its functionality you need to have it as the part of the kernel to >> be to get console, mmc, ethernet, usb, etc. >> >> That's why I would like to know what functionality and use case you have >> in mind that this driver should be made module. > > The "functionality" of building a kernel image that works on all > hardware types. Just like x86-64 has been for a very long time :) ok. Thanks, Michal
On Wed, Mar 24, 2021 at 10:02:53AM +0100, Michal Simek wrote: > > > On 3/24/21 9:49 AM, Greg Kroah-Hartman wrote: > > On Wed, Mar 24, 2021 at 09:29:12AM +0100, Michal Simek wrote: > >> On 3/23/21 2:42 PM, Greg Kroah-Hartman wrote: > >>> On Wed, Mar 17, 2021 at 01:55:16PM +0530, Sai Krishna Potthuri wrote: > >>>> Adding pinctrl driver for Xilinx ZynqMP platform. > >>>> This driver queries pin information from firmware and registers > >>>> pin control accordingly. > >>>> > >>>> Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> > >>>> --- > >>>> drivers/pinctrl/Kconfig | 13 + > >>>> drivers/pinctrl/Makefile | 1 + > >>>> drivers/pinctrl/pinctrl-zynqmp.c | 1030 ++++++++++++++++++++++++++++++ > >>>> 3 files changed, 1044 insertions(+) > >>>> create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c > >>>> > >>>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > >>>> index 815095326e2d..25d3c7208975 100644 > >>>> --- a/drivers/pinctrl/Kconfig > >>>> +++ b/drivers/pinctrl/Kconfig > >>>> @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ > >>>> help > >>>> This selects the pinctrl driver for Xilinx Zynq. > >>>> > >>>> +config PINCTRL_ZYNQMP > >>>> + bool "Pinctrl driver for Xilinx ZynqMP" > >>> > >>> Please make this work as a module. > >> > >> The most of pinctrl drivers are builtin modules now which is not excuse > >> it is just fact. > >> $ git grep module_pla drivers/pinctrl/ | wc -l > >> 40 > >> $ git grep builtin_pla drivers/pinctrl/ | wc -l > >> 64 > > > > For new ones, we can do better, don't make us have to go back and fix > > this up later. > > As I said not a big deal. If this is the way to go then I these rules > should be followed which is not what it is happening based on 3 latest > pinctrl drivers below. I do not disagree, but I point out issues when I see them, you got unlucky :)
On 3/24/21 10:13 AM, Greg Kroah-Hartman wrote: > On Wed, Mar 24, 2021 at 10:02:53AM +0100, Michal Simek wrote: >> >> >> On 3/24/21 9:49 AM, Greg Kroah-Hartman wrote: >>> On Wed, Mar 24, 2021 at 09:29:12AM +0100, Michal Simek wrote: >>>> On 3/23/21 2:42 PM, Greg Kroah-Hartman wrote: >>>>> On Wed, Mar 17, 2021 at 01:55:16PM +0530, Sai Krishna Potthuri wrote: >>>>>> Adding pinctrl driver for Xilinx ZynqMP platform. >>>>>> This driver queries pin information from firmware and registers >>>>>> pin control accordingly. >>>>>> >>>>>> Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> >>>>>> --- >>>>>> drivers/pinctrl/Kconfig | 13 + >>>>>> drivers/pinctrl/Makefile | 1 + >>>>>> drivers/pinctrl/pinctrl-zynqmp.c | 1030 ++++++++++++++++++++++++++++++ >>>>>> 3 files changed, 1044 insertions(+) >>>>>> create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c >>>>>> >>>>>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig >>>>>> index 815095326e2d..25d3c7208975 100644 >>>>>> --- a/drivers/pinctrl/Kconfig >>>>>> +++ b/drivers/pinctrl/Kconfig >>>>>> @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ >>>>>> help >>>>>> This selects the pinctrl driver for Xilinx Zynq. >>>>>> >>>>>> +config PINCTRL_ZYNQMP >>>>>> + bool "Pinctrl driver for Xilinx ZynqMP" >>>>> >>>>> Please make this work as a module. >>>> >>>> The most of pinctrl drivers are builtin modules now which is not excuse >>>> it is just fact. >>>> $ git grep module_pla drivers/pinctrl/ | wc -l >>>> 40 >>>> $ git grep builtin_pla drivers/pinctrl/ | wc -l >>>> 64 >>> >>> For new ones, we can do better, don't make us have to go back and fix >>> this up later. >> >> As I said not a big deal. If this is the way to go then I these rules >> should be followed which is not what it is happening based on 3 latest >> pinctrl drivers below. > > I do not disagree, but I point out issues when I see them, you got > unlucky :) I feel we were lucky that our driver got your attention and we do it properly from the beginning. :-) Thanks, Michal
On Mon, Mar 22, 2021 at 4:25 PM Sai Krishna Potthuri <lakshmis@xilinx.com> wrote: > [Andy] > > > > > + PIN_CONFIG_IOSTANDARD = PIN_CONFIG_END + 1, }; > > > > > > > > I'm lost here. What is IO standard exactly? Why it can't be in > > > > generic headers? > > > It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts. > > > Since this is specific to Xilinx ZynqMP platform, considered to be > > > added in the driver file. > > > > So, why can't we create a couple of bits to represent this voltages in the > > generic header and gain usability for others as well? > > I see some drivers are maintaining the configuration list in the driver file, if > the configuration is specific to the driver. > > We can move this to generic header if it is used by others as well. > Ok, will wait for Linus to comment. > > > > Linus? While it is fine to add custom pin config options to pin controllers for hopelessly idiomatic stuff, this does look like it should be using PIN_CONFIG_POWER_SOURCE with the voltage rail as parameter, see include/linux/pinctrl/pinconf-generic.h If you're not using that then tell us why. Yours, Linus Walleij
Hi Linus, > -----Original Message----- > From: Linus Walleij <linus.walleij@linaro.org> > Sent: Thursday, March 25, 2021 2:27 PM > To: Sai Krishna Potthuri <lakshmis@xilinx.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Rob Herring > <robh+dt@kernel.org>; Michal Simek <michals@xilinx.com>; Greg Kroah- > Hartman <gregkh@linuxfoundation.org>; linux-arm Mailing List <linux-arm- > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; devicetree <devicetree@vger.kernel.org>; open > list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>; git <git@xilinx.com>; > saikrishna12468@gmail.com > Subject: Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support > > On Mon, Mar 22, 2021 at 4:25 PM Sai Krishna Potthuri > <lakshmis@xilinx.com> wrote: > > [Andy] > > > > > > > + PIN_CONFIG_IOSTANDARD = PIN_CONFIG_END + 1, }; > > > > > > > > > > I'm lost here. What is IO standard exactly? Why it can't be in > > > > > generic headers? > > > > It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts. > > > > Since this is specific to Xilinx ZynqMP platform, considered to be > > > > added in the driver file. > > > > > > So, why can't we create a couple of bits to represent this voltages > > > in the generic header and gain usability for others as well? > > > > I see some drivers are maintaining the configuration list in the > > driver file, if the configuration is specific to the driver. > > > > We can move this to generic header if it is used by others as well. > > Ok, will wait for Linus to comment. > > > > > > Linus? > > While it is fine to add custom pin config options to pin controllers for > hopelessly idiomatic stuff, this does look like it should be using > PIN_CONFIG_POWER_SOURCE with the voltage rail as parameter, see > include/linux/pinctrl/pinconf-generic.h Thanks. I will update the driver to use 'power-source' option. Regards Sai Krishna > > If you're not using that then tell us why. > > Yours, > Linus Walleij
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 815095326e2d..25d3c7208975 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -341,6 +341,19 @@ config PINCTRL_ZYNQ help This selects the pinctrl driver for Xilinx Zynq. +config PINCTRL_ZYNQMP + bool "Pinctrl driver for Xilinx ZynqMP" + depends on ARCH_ZYNQMP + select PINMUX + select GENERIC_PINCONF + help + This selects the pinctrl driver for Xilinx ZynqMP platform. + This driver will query the pin information from the firmware + and allow configuring the pins. + Configuration can include the mux function to select on those + pin(s)/group(s), and various pin configuration parameters + such as pull-up, slew rate, etc. + config PINCTRL_INGENIC bool "Pinctrl driver for the Ingenic JZ47xx SoCs" default MACH_INGENIC diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index f53933b2ff02..7e058739f0d5 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -43,6 +43,7 @@ obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o +obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o obj-$(CONFIG_PINCTRL_INGENIC) += pinctrl-ingenic.o obj-$(CONFIG_PINCTRL_RK805) += pinctrl-rk805.o obj-$(CONFIG_PINCTRL_OCELOT) += pinctrl-ocelot.o diff --git a/drivers/pinctrl/pinctrl-zynqmp.c b/drivers/pinctrl/pinctrl-zynqmp.c new file mode 100644 index 000000000000..63edde400e85 --- /dev/null +++ b/drivers/pinctrl/pinctrl-zynqmp.c @@ -0,0 +1,1030 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ZynqMP pin controller + * + * Copyright (C) 2020 Xilinx, Inc. + * + * Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> + * Rajan Vaja <rajanv@xilinx.com> + */ + +#include <linux/init.h> +#include <linux/of_address.h> +#include <linux/pinctrl/pinmux.h> +#include <linux/pinctrl/pinconf-generic.h> +#include <dt-bindings/pinctrl/pinctrl-zynqmp.h> +#include <linux/platform_device.h> +#include <linux/firmware/xlnx-zynqmp.h> +#include "core.h" +#include "pinctrl-utils.h" + +#define ZYNQMP_PIN_PREFIX "MIO" +#define PINCTRL_GET_FUNC_NAME_RESP_LEN 16 +#define MAX_FUNC_NAME_LEN 16 +#define MAX_GROUP_PIN 50 +#define END_OF_FUNCTIONS "END_OF_FUNCTIONS" +#define NUM_GROUPS_PER_RESP 6 + +#define PINCTRL_GET_FUNC_GROUPS_RESP_LEN 12 +#define PINCTRL_GET_PIN_GROUPS_RESP_LEN 12 +#define NA_GROUP -1 +#define RESERVED_GROUP -2 + +#define DRIVE_STRENGTH_2MA 2 +#define DRIVE_STRENGTH_4MA 4 +#define DRIVE_STRENGTH_8MA 8 +#define DRIVE_STRENGTH_12MA 12 + +/** + * struct zynqmp_pmux_function - a pinmux function + * @name: Name of the pinmux function + * @groups: List of pingroups for this function + * @ngroups: Number of entries in @groups + * @node:` Firmware node matching with for function + * + * This structure holds information about pin control function + * and function group names supporting that function. + */ +struct zynqmp_pmux_function { + char name[MAX_FUNC_NAME_LEN]; + const char * const *groups; + unsigned int ngroups; +}; + +/** + * struct zynqmp_pinctrl - driver data + * @pctrl: Pinctrl device + * @groups: Pingroups + * @ngroups: Number of @groups + * @funcs: Pinmux functions + * @nfuncs: Number of @funcs + * + * This struct is stored as driver data and used to retrieve + * information regarding pin control functions, groups and + * group pins. + */ +struct zynqmp_pinctrl { + struct pinctrl_dev *pctrl; + const struct zynqmp_pctrl_group *groups; + unsigned int ngroups; + const struct zynqmp_pmux_function *funcs; + unsigned int nfuncs; +}; + +/** + * struct zynqmp_pctrl_group - Pin control group info + * @name: Group name + * @pins: Group pin numbers + * @npins: Number of pins in group + */ +struct zynqmp_pctrl_group { + const char *name; + unsigned int pins[MAX_GROUP_PIN]; + unsigned int npins; +}; + +/** + * enum zynqmp_pin_config_param - possible pin configuration parameters + * @PIN_CONFIG_IOSTANDARD: if the pin can select an IO standard, + * the argument to this parameter (on a + * custom format) tells the driver which + * alternative IO standard to use + */ +enum zynqmp_pin_config_param { + PIN_CONFIG_IOSTANDARD = PIN_CONFIG_END + 1, +}; + +static const struct pinconf_generic_params zynqmp_dt_params[] = { + {"io-standard", PIN_CONFIG_IOSTANDARD, IO_STANDARD_LVCMOS18}, +}; + +#ifdef CONFIG_DEBUG_FS +static const struct +pin_config_item zynqmp_conf_items[ARRAY_SIZE(zynqmp_dt_params)] = { + PCONFDUMP(PIN_CONFIG_IOSTANDARD, "io-standard", NULL, true), +}; +#endif + +static struct pinctrl_desc zynqmp_desc; + +/** + * zynqmp_pctrl_get_groups_count() - get group count + * @pctldev: Pincontrol device pointer. + * + * Get total groups count. + * + * Return: group count. + */ +static int zynqmp_pctrl_get_groups_count(struct pinctrl_dev *pctldev) +{ + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + + return pctrl->ngroups; +} + +/** + * zynqmp_pctrl_get_group_name() - get group name + * @pctldev: Pincontrol device pointer. + * @selector: Group ID. + * + * Get gorup's name. + * + * Return: group name. + */ +static const char *zynqmp_pctrl_get_group_name(struct pinctrl_dev *pctldev, + unsigned int selector) +{ + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + + return pctrl->groups[selector].name; +} + +/** + * zynqmp_pctrl_get_group_pins() - get group pins + * @pctldev: Pincontrol device pointer. + * @selector: Group ID. + * @pins: Pin numbers. + * @npins: Number of pins in group. + * + * Get gorup's pin count and pin number. + * + * Return: Success. + */ +static int zynqmp_pctrl_get_group_pins(struct pinctrl_dev *pctldev, + unsigned int selector, + const unsigned int **pins, + unsigned int *npins) +{ + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + + *pins = pctrl->groups[selector].pins; + *npins = pctrl->groups[selector].npins; + + return 0; +} + +static const struct pinctrl_ops zynqmp_pctrl_ops = { + .get_groups_count = zynqmp_pctrl_get_groups_count, + .get_group_name = zynqmp_pctrl_get_group_name, + .get_group_pins = zynqmp_pctrl_get_group_pins, + .dt_node_to_map = pinconf_generic_dt_node_to_map_all, + .dt_free_map = pinctrl_utils_free_map, +}; + +/** + * zynqmp_pinmux_request_pin() - Request a pin for muxing + * @pctldev: Pincontrol device pointer. + * @pin: Pin number. + * + * Request a pin from firmware for muxing. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinmux_request_pin(struct pinctrl_dev *pctldev, + unsigned int pin) +{ + int ret; + + ret = zynqmp_pm_pinctrl_request(pin); + if (ret) { + dev_err(pctldev->dev, "request failed for pin %u\n", pin); + return -EIO; + } + + return 0; +} + +/** + * zynqmp_pmux_get_functions_count() - get number of functions + * @pctldev: Pincontrol device pointer. + * + * Get total function count. + * + * Return: function count. + */ +static int zynqmp_pmux_get_functions_count(struct pinctrl_dev *pctldev) +{ + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + + return pctrl->nfuncs; +} + +/** + * zynqmp_pmux_get_function_name() - get function name + * @pctldev: Pincontrol device pointer. + * @selector: Function ID. + * + * Get function's name. + * + * Return: function name. + */ +static const char *zynqmp_pmux_get_function_name(struct pinctrl_dev *pctldev, + unsigned int selector) +{ + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + + return pctrl->funcs[selector].name; +} + +/** + * zynqmp_pmux_get_function_groups() - Get groups for the function + * @pctldev: Pincontrol device pointer. + * @selector: Function ID + * @groups: Group names. + * @num_groups: Number of function groups. + * + * Get function's group count and group names. + * + * Return: Success. + */ +static int zynqmp_pmux_get_function_groups(struct pinctrl_dev *pctldev, + unsigned int selector, + const char * const **groups, + unsigned * const num_groups) +{ + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + + *groups = pctrl->funcs[selector].groups; + *num_groups = pctrl->funcs[selector].ngroups; + + return 0; +} + +/** + * zynqmp_pinmux_set_mux() - Set requested function for the group + * @pctldev: Pincontrol device pointer. + * @function: Function ID. + * @group: Group ID. + * + * Loop though all pins of group and call firmware API + * to set requested function for all pins in group. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinmux_set_mux(struct pinctrl_dev *pctldev, + unsigned int function, + unsigned int group) +{ + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + const struct zynqmp_pctrl_group *pgrp = &pctrl->groups[group]; + int ret, i; + + for (i = 0; i < pgrp->npins; i++) { + unsigned int pin = pgrp->pins[i]; + + ret = zynqmp_pm_pinctrl_set_function(pin, function); + if (ret) { + dev_err(pctldev->dev, "set mux failed for pin %u\n", + pin); + return -EIO; + } + } + + return 0; +} + +/** + * zynqmp_pinmux_release_pin() - Release a pin + * @pctldev: Pincontrol device pointer. + * @pin: Pin number. + * + * Release a pin from firmware. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinmux_release_pin(struct pinctrl_dev *pctldev, + unsigned int pin) +{ + int ret; + + ret = zynqmp_pm_pinctrl_release(pin); + if (ret) { + dev_err(pctldev->dev, "free pin failed for pin %u\n", + pin); + return -EIO; + } + + return 0; +} + +static const struct pinmux_ops zynqmp_pinmux_ops = { + .request = zynqmp_pinmux_request_pin, + .get_functions_count = zynqmp_pmux_get_functions_count, + .get_function_name = zynqmp_pmux_get_function_name, + .get_function_groups = zynqmp_pmux_get_function_groups, + .set_mux = zynqmp_pinmux_set_mux, + .free = zynqmp_pinmux_release_pin, +}; + +/** + * zynqmp_pinconf_cfg_get() - get config value for the pin + * @pctldev: Pin control device pointer. + * @pin: Pin number. + * @config: Value of config param. + * + * Get value of the requested configuration parameter for the + * given pin. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinconf_cfg_get(struct pinctrl_dev *pctldev, + unsigned int pin, + unsigned long *config) +{ + int ret; + unsigned int arg = 0, param = pinconf_to_config_param(*config); + + if (pin >= zynqmp_desc.npins) + return -EOPNOTSUPP; + + switch (param) { + case PIN_CONFIG_SLEW_RATE: + param = PM_PINCTRL_CONFIG_SLEW_RATE; + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); + break; + case PIN_CONFIG_BIAS_PULL_UP: + param = PM_PINCTRL_CONFIG_PULL_CTRL; + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); + if (arg != PM_PINCTRL_BIAS_PULL_UP) + return -EINVAL; + + arg = 1; + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + param = PM_PINCTRL_CONFIG_PULL_CTRL; + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); + if (arg != PM_PINCTRL_BIAS_PULL_DOWN) + return -EINVAL; + + arg = 1; + break; + case PIN_CONFIG_BIAS_DISABLE: + param = PM_PINCTRL_CONFIG_BIAS_STATUS; + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); + if (arg != PM_PINCTRL_BIAS_DISABLE) + return -EINVAL; + + arg = 1; + break; + case PIN_CONFIG_IOSTANDARD: + param = PM_PINCTRL_CONFIG_VOLTAGE_STATUS; + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); + break; + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: + param = PM_PINCTRL_CONFIG_SCHMITT_CMOS; + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); + break; + case PIN_CONFIG_DRIVE_STRENGTH: + param = PM_PINCTRL_CONFIG_DRIVE_STRENGTH; + ret = zynqmp_pm_pinctrl_get_config(pin, param, &arg); + switch (arg) { + case PM_PINCTRL_DRIVE_STRENGTH_2MA: + arg = DRIVE_STRENGTH_2MA; + break; + case PM_PINCTRL_DRIVE_STRENGTH_4MA: + arg = DRIVE_STRENGTH_4MA; + break; + case PM_PINCTRL_DRIVE_STRENGTH_8MA: + arg = DRIVE_STRENGTH_8MA; + break; + case PM_PINCTRL_DRIVE_STRENGTH_12MA: + arg = DRIVE_STRENGTH_12MA; + break; + default: + /* Invalid drive strength */ + dev_warn(pctldev->dev, + "Invalid drive strength for pin %d\n", + pin); + return -EINVAL; + } + break; + default: + ret = -EOPNOTSUPP; + break; + } + + param = pinconf_to_config_param(*config); + *config = pinconf_to_config_packed(param, arg); + + return ret; +} + +/** + * zynqmp_pinconf_cfg_set() - Set requested config for the pin + * @pctldev: Pincontrol device pointer. + * @pin: Pin number. + * @configs: Configuration to set. + * @num_configs: Number of configurations. + * + * Loop though all configurations and call firmware API + * to set requested configurations for the pin. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinconf_cfg_set(struct pinctrl_dev *pctldev, + unsigned int pin, unsigned long *configs, + unsigned int num_configs) +{ + int i, ret; + + if (pin >= zynqmp_desc.npins) + return -EOPNOTSUPP; + + for (i = 0; i < num_configs; i++) { + unsigned int param = pinconf_to_config_param(configs[i]); + unsigned int arg = pinconf_to_config_argument(configs[i]); + unsigned int value; + + switch (param) { + case PIN_CONFIG_SLEW_RATE: + param = PM_PINCTRL_CONFIG_SLEW_RATE; + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); + break; + case PIN_CONFIG_BIAS_PULL_UP: + param = PM_PINCTRL_CONFIG_PULL_CTRL; + arg = PM_PINCTRL_BIAS_PULL_UP; + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + param = PM_PINCTRL_CONFIG_PULL_CTRL; + arg = PM_PINCTRL_BIAS_PULL_DOWN; + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); + break; + case PIN_CONFIG_BIAS_DISABLE: + param = PM_PINCTRL_CONFIG_BIAS_STATUS; + arg = PM_PINCTRL_BIAS_DISABLE; + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); + break; + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: + param = PM_PINCTRL_CONFIG_SCHMITT_CMOS; + ret = zynqmp_pm_pinctrl_set_config(pin, param, arg); + break; + case PIN_CONFIG_DRIVE_STRENGTH: + switch (arg) { + case DRIVE_STRENGTH_2MA: + value = PM_PINCTRL_DRIVE_STRENGTH_2MA; + break; + case DRIVE_STRENGTH_4MA: + value = PM_PINCTRL_DRIVE_STRENGTH_4MA; + break; + case DRIVE_STRENGTH_8MA: + value = PM_PINCTRL_DRIVE_STRENGTH_8MA; + break; + case DRIVE_STRENGTH_12MA: + value = PM_PINCTRL_DRIVE_STRENGTH_12MA; + break; + default: + /* Invalid drive strength */ + dev_warn(pctldev->dev, + "Invalid drive strength for pin %d\n", + pin); + return -EINVAL; + } + + param = PM_PINCTRL_CONFIG_DRIVE_STRENGTH; + ret = zynqmp_pm_pinctrl_set_config(pin, param, value); + break; + case PIN_CONFIG_IOSTANDARD: + param = PM_PINCTRL_CONFIG_VOLTAGE_STATUS; + ret = zynqmp_pm_pinctrl_get_config(pin, param, &value); + + if (arg != value) + dev_warn(pctldev->dev, + "Invalid IO Standard requested for pin %d\n", + pin); + + break; + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE: + case PIN_CONFIG_LOW_POWER_MODE: + /* + * These cases are mentioned in dts but configurable + * registers are unknown. So falling through to ignore + * boot time warnings as of now. + */ + ret = 0; + break; + default: + dev_warn(pctldev->dev, + "unsupported configuration parameter '%u'\n", + param); + ret = -EOPNOTSUPP; + break; + } + + param = pinconf_to_config_param(configs[i]); + arg = pinconf_to_config_argument(configs[i]); + if (ret) + dev_warn(pctldev->dev, + "%s failed: pin %u param %u value %u\n", + __func__, pin, param, arg); + } + + return 0; +} + +/** + * zynqmp_pinconf_group_set() - Set requested config for the group + * @pctldev: Pincontrol device pointer. + * @selector: Group ID. + * @configs: Configuration to set. + * @num_configs: Number of configurations. + * + * Call function to set configs for each pin in group. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinconf_group_set(struct pinctrl_dev *pctldev, + unsigned int selector, + unsigned long *configs, + unsigned int num_configs) +{ + int i, ret; + struct zynqmp_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + const struct zynqmp_pctrl_group *pgrp = &pctrl->groups[selector]; + + for (i = 0; i < pgrp->npins; i++) { + ret = zynqmp_pinconf_cfg_set(pctldev, pgrp->pins[i], configs, + num_configs); + if (ret) + return ret; + } + + return 0; +} + +static const struct pinconf_ops zynqmp_pinconf_ops = { + .is_generic = true, + .pin_config_get = zynqmp_pinconf_cfg_get, + .pin_config_set = zynqmp_pinconf_cfg_set, + .pin_config_group_set = zynqmp_pinconf_group_set, +}; + +static struct pinctrl_desc zynqmp_desc = { + .name = "zynqmp_pinctrl", + .owner = THIS_MODULE, + .pctlops = &zynqmp_pctrl_ops, + .pmxops = &zynqmp_pinmux_ops, + .confops = &zynqmp_pinconf_ops, +#ifdef CONFIG_DEBUG_FS + .custom_conf_items = zynqmp_conf_items, +#endif +}; + +/** + * zynqmp_pinctrl_get_function_groups() - get groups for the function + * @fid: Function ID. + * @index: Group index. + * @groups: Groups data. + * + * Call firmware API to get groups for the given function. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_get_function_groups(u32 fid, u32 index, u16 *groups) +{ + struct zynqmp_pm_query_data qdata = {0}; + u32 ret_payload[PAYLOAD_ARG_CNT]; + int ret; + + qdata.qid = PM_QID_PINCTRL_GET_FUNCTION_GROUPS; + qdata.arg1 = fid; + qdata.arg2 = index; + + ret = zynqmp_pm_query_data(qdata, ret_payload); + if (ret) + return ret; + + memcpy(groups, &ret_payload[1], PINCTRL_GET_FUNC_GROUPS_RESP_LEN); + + return ret; +} + +/** + * zynqmp_pinctrl_get_func_num_groups() - get number of groups in function + * @fid: Function ID. + * @ngroups: Number of groups in function. + * + * Call firmware API to get number of group in function. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_get_func_num_groups(u32 fid, unsigned int *ngroups) +{ + struct zynqmp_pm_query_data qdata = {0}; + u32 ret_payload[PAYLOAD_ARG_CNT]; + int ret; + + qdata.qid = PM_QID_PINCTRL_GET_NUM_FUNCTION_GROUPS; + qdata.arg1 = fid; + + ret = zynqmp_pm_query_data(qdata, ret_payload); + if (ret) + return ret; + + *ngroups = ret_payload[1]; + + return ret; +} + +/** + * zynqmp_pinctrl_prepare_func_groups() - prepare function and groups data + * @dev: Device pointer. + * @fid: Function ID. + * @func: Function data. + * @groups: Groups data. + * + * Query firmware to get group IDs for each function. Firmware returns + * group IDs. Based on gorup index for the function, group names in + * function are stored. For example, first gorup in "eth0" function + * is named as "eth0_0", second as "eth0_1" and so on. + * + * Based on group ID received from firmware, function stores name of + * group for that group ID. For an example, if "eth0" first group ID + * is x, groups[x] name will be stored as "eth0_0". + * + * Once done for each function, each function would have its group names, + * and each groups would also have their names. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_prepare_func_groups(struct device *dev, u32 fid, + struct zynqmp_pmux_function *func, + struct zynqmp_pctrl_group *groups) +{ + u16 resp[NUM_GROUPS_PER_RESP] = {0}; + const char **fgroups; + int ret = 0, index, i; + + fgroups = devm_kzalloc(dev, sizeof(*fgroups) * func->ngroups, + GFP_KERNEL); + if (!fgroups) + return -ENOMEM; + + for (index = 0; index < func->ngroups; index += NUM_GROUPS_PER_RESP) { + ret = zynqmp_pinctrl_get_function_groups(fid, index, resp); + if (ret) + return ret; + + for (i = 0; i < NUM_GROUPS_PER_RESP; i++) { + if (resp[i] == (u16)NA_GROUP) + goto done; + + if (resp[i] == (u16)RESERVED_GROUP) + continue; + + fgroups[index + i] = devm_kasprintf(dev, GFP_KERNEL, + "%s_%d_grp", + func->name, + index + i); + groups[resp[i]].name = devm_kasprintf(dev, GFP_KERNEL, + "%s_%d_grp", + func->name, + index + i); + } + } +done: + func->groups = fgroups; + + return ret; +} + +/** + * zynqmp_pinctrl_get_function_name() - get function name + * @fid: Function ID. + * @name: Function name + * + * Call firmware API to get name of given function. + */ +static void zynqmp_pinctrl_get_function_name(u32 fid, char *name) +{ + struct zynqmp_pm_query_data qdata = {0}; + u32 ret_payload[PAYLOAD_ARG_CNT]; + + qdata.qid = PM_QID_PINCTRL_GET_FUNCTION_NAME; + qdata.arg1 = fid; + + /* + * Name of the function is maximum 16 bytes and cannot + * accommodate the return value in SMC buffers, hence ignoring + * the return value for this specific qid. + */ + zynqmp_pm_query_data(qdata, ret_payload); + memcpy(name, ret_payload, PINCTRL_GET_FUNC_NAME_RESP_LEN); +} + +/** + * zynqmp_pinctrl_get_num_functions() - get number of supported functions + * @nfuncs: Number of functions. + * + * Call firmware API to get number of functions supported by system/board. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_get_num_functions(unsigned int *nfuncs) +{ + struct zynqmp_pm_query_data qdata = {0}; + u32 ret_payload[PAYLOAD_ARG_CNT]; + int ret; + + qdata.qid = PM_QID_PINCTRL_GET_NUM_FUNCTIONS; + + ret = zynqmp_pm_query_data(qdata, ret_payload); + if (ret) + return ret; + + *nfuncs = ret_payload[1]; + + return ret; +} + +/** + * zynqmp_pinctrl_get_pin_groups() - get groups for the pin + * @pin: Pin number. + * @index: Group index. + * @groups: Groups data. + * + * Call firmware API to get groups for the given pin. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_get_pin_groups(u32 pin, u32 index, u16 *groups) +{ + struct zynqmp_pm_query_data qdata = {0}; + u32 ret_payload[PAYLOAD_ARG_CNT]; + int ret; + + qdata.qid = PM_QID_PINCTRL_GET_PIN_GROUPS; + qdata.arg1 = pin; + qdata.arg2 = index; + + ret = zynqmp_pm_query_data(qdata, ret_payload); + if (ret) + return ret; + + memcpy(groups, &ret_payload[1], PINCTRL_GET_PIN_GROUPS_RESP_LEN); + + return ret; +} + +/** + * zynqmp_pinctrl_group_add_pin() - add pin to given group + * @group: Group data. + * @pin: Pin number. + * + * Add pin number to respective group's pin array at end and + * increment pin count for the group. + */ +static void zynqmp_pinctrl_group_add_pin(struct zynqmp_pctrl_group *group, + unsigned int pin) +{ + group->pins[group->npins++] = pin; +} + +/** + * zynqmp_pinctrl_create_pin_groups() - assign pins to respective groups + * @dev: Device pointer. + * @groups: Groups data. + * @pin: Pin number. + * + * Query firmware to get groups available for the given pin. + * Based on firmware response(group IDs for the pin), add + * pin number to respective group's pin array. + * + * Once all pins are queries, each groups would have its number + * of pins and pin numbers data. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_create_pin_groups(struct device *dev, + struct zynqmp_pctrl_group *groups, + unsigned int pin) +{ + int ret, i, index = 0; + u16 resp[NUM_GROUPS_PER_RESP] = {0}; + + do { + ret = zynqmp_pinctrl_get_pin_groups(pin, index, resp); + if (ret) + return ret; + + for (i = 0; i < NUM_GROUPS_PER_RESP; i++) { + if (resp[i] == (u16)NA_GROUP) + return ret; + + if (resp[i] == (u16)RESERVED_GROUP) + continue; + + zynqmp_pinctrl_group_add_pin(&groups[resp[i]], pin); + } + index += NUM_GROUPS_PER_RESP; + } while (1); + + return ret; +} + +/** + * zynqmp_pinctrl_prepare_group_pins() - prepare each group's pin data + * @dev: Device pointer. + * @groups: Groups data. + * @ngroups: Number of groups. + * + * Prepare pin number and number of pins data for each pins. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_prepare_group_pins(struct device *dev, + struct zynqmp_pctrl_group *groups, + unsigned int ngroups) +{ + unsigned int pin; + int ret = 0; + + for (pin = 0; pin < zynqmp_desc.npins; pin++) { + ret = zynqmp_pinctrl_create_pin_groups(dev, groups, pin); + if (ret) + return ret; + } + + return ret; +} + +/** + * zynqmp_pinctrl_prepare_function_info() - prepare function info + * @dev: Device pointer. + * @pctrl: Pin control driver data. + * + * Query firmware for functions, groups and pin information and + * prepare pin control driver data. + * + * Query number of functions and number of function groups (number + * of groups in given function) to allocate required memory buffers + * for functions and groups. Once buffers are allocated to store + * functions and groups data, query and store required information + * (numbe of groups and group names for each function, number of + * pins and pin numbers for each group). + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_prepare_function_info(struct device *dev, + struct zynqmp_pinctrl *pctrl) +{ + struct zynqmp_pmux_function *funcs; + struct zynqmp_pctrl_group *groups; + int ret, i; + + ret = zynqmp_pinctrl_get_num_functions(&pctrl->nfuncs); + if (ret) + return ret; + + funcs = devm_kzalloc(dev, sizeof(*funcs) * pctrl->nfuncs, GFP_KERNEL); + if (!funcs) + return -ENOMEM; + + for (i = 0; i < pctrl->nfuncs; i++) { + zynqmp_pinctrl_get_function_name(i, funcs[i].name); + + ret = zynqmp_pinctrl_get_func_num_groups(i, &funcs[i].ngroups); + if (ret) + return ret; + + pctrl->ngroups += funcs[i].ngroups; + } + + groups = devm_kzalloc(dev, sizeof(*groups) * pctrl->ngroups, + GFP_KERNEL); + if (!groups) + return -ENOMEM; + + for (i = 0; i < pctrl->nfuncs; i++) { + ret = zynqmp_pinctrl_prepare_func_groups(dev, i, &funcs[i], + groups); + if (ret) + return ret; + } + + ret = zynqmp_pinctrl_prepare_group_pins(dev, groups, pctrl->ngroups); + if (ret) + return ret; + + pctrl->funcs = funcs; + pctrl->groups = groups; + + return ret; +} + +/** + * zynqmp_pinctrl_get_num_pins() - get number of pins in system + * @npins: Number of pins in system/board. + * + * Call firmware API to get number of pins. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_get_num_pins(unsigned int *npins) +{ + struct zynqmp_pm_query_data qdata = {0}; + u32 ret_payload[PAYLOAD_ARG_CNT]; + int ret; + + qdata.qid = PM_QID_PINCTRL_GET_NUM_PINS; + + ret = zynqmp_pm_query_data(qdata, ret_payload); + if (ret) + return ret; + + *npins = ret_payload[1]; + + return ret; +} + +/** + * zynqmp_pinctrl_prepare_pin_desc() - prepare pin description info + * @dev: Device pointer. + * @zynqmp_pins: Pin information. + * @npins: Number of pins. + * + * Query number of pins information from firmware and prepare pin + * description containing pin number and pin name. + * + * Return: 0 on success else error code. + */ +static int zynqmp_pinctrl_prepare_pin_desc(struct device *dev, + const struct pinctrl_pin_desc + **zynqmp_pins, + unsigned int *npins) +{ + struct pinctrl_pin_desc *pins, *pin; + int ret; + int i; + + ret = zynqmp_pinctrl_get_num_pins(npins); + if (ret) + return ret; + + pins = devm_kzalloc(dev, sizeof(*pins) * *npins, GFP_KERNEL); + if (!pins) + return -ENOMEM; + + for (i = 0; i < *npins; i++) { + pin = &pins[i]; + pin->number = i; + pin->name = devm_kasprintf(dev, GFP_KERNEL, "%s%d", + ZYNQMP_PIN_PREFIX, i); + } + + *zynqmp_pins = pins; + + return 0; +} + +static int zynqmp_pinctrl_probe(struct platform_device *pdev) +{ + struct zynqmp_pinctrl *pctrl; + int ret; + + pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL); + if (!pctrl) + return -ENOMEM; + + ret = zynqmp_pinctrl_prepare_pin_desc(&pdev->dev, + &zynqmp_desc.pins, + &zynqmp_desc.npins); + if (ret) { + dev_err(&pdev->dev, "%s() pin desc prepare fail with %d\n", + __func__, ret); + return ret; + } + + ret = zynqmp_pinctrl_prepare_function_info(&pdev->dev, pctrl); + if (ret) { + dev_err(&pdev->dev, "%s() function info prepare fail with %d\n", + __func__, ret); + return ret; + } + + pctrl->pctrl = pinctrl_register(&zynqmp_desc, &pdev->dev, pctrl); + if (IS_ERR(pctrl->pctrl)) { + ret = PTR_ERR(pctrl->pctrl); + return ret; + } + + platform_set_drvdata(pdev, pctrl); + + dev_info(&pdev->dev, "zynqmp pinctrl initialized\n"); + + return ret; +} + +static const struct of_device_id zynqmp_pinctrl_of_match[] = { + { .compatible = "xlnx,zynqmp-pinctrl" }, + { } +}; + +static struct platform_driver zynqmp_pinctrl_driver = { + .driver = { + .name = "zynqmp-pinctrl", + .of_match_table = zynqmp_pinctrl_of_match, + }, + .probe = zynqmp_pinctrl_probe, +}; +builtin_platform_driver(zynqmp_pinctrl_driver);
Adding pinctrl driver for Xilinx ZynqMP platform. This driver queries pin information from firmware and registers pin control accordingly. Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com> --- drivers/pinctrl/Kconfig | 13 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-zynqmp.c | 1030 ++++++++++++++++++++++++++++++ 3 files changed, 1044 insertions(+) create mode 100644 drivers/pinctrl/pinctrl-zynqmp.c