* [RFC 0/5] Renesas RZ series pinctrl driver @ 2017-01-25 18:09 Jacopo Mondi 2017-01-25 18:09 ` [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module Jacopo Mondi ` (8 more replies) 0 siblings, 9 replies; 54+ messages in thread From: Jacopo Mondi @ 2017-01-25 18:09 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hello, after having discussed in great detail the RZ series per-pin PFC hardware peculiarities, this is a proposal for a possible pin-based pin controller driver for SoC devices of Renesas RZ family. This RFC series adds a minimal driver infrastructure which supports pin multiplexing via explicit per-pin settings performed in device tree sources. The driver is composed by a "core" module, which aims to be generic enough to support different RZ SoC device, and a "SoC" module, which is instead specific to the single device. Right now, the only "SoC" module support implemented is for RZ/A1H (Genmai and GR-Peach boards). Why an "SoC" module, if a single SoC is supported then? RZ devices with a pin-based PFC hardware have different register layouts and available pin functions, the "SoC" module is requested to enumerate its pins, and provide function for HW interfacing (currently only set_mux() is supported). This should make adding support for new chips fairly easy. One note on the current DT ABI: right now a pin configuration is specified in DTS using utility macros defined in the (currently undocumented) arch/arm/boot/dts/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h header file. Each pin configuration is a triplet of u32 in the form of <BANK PIN ALTERNATE_FUNC_#> It should be fairly easy adding additional parameters to configure what was missing in the original group-based PFC driver for RZ devices (I'm thinking of IO mode control, input buffer configuration, bi-directional configuration etc). Once these configuration parameters have been retrieved from the DTS, they can be passed down to the "SoC" module, and let it deal with them as it does right now with mux settings. Currently, there is no support for pinconfig operations, and no GPIO support. I'm planning to work on GPIO integration as soon as I have sent this out, but I wanted to send this first to start collecting comments as soon as I could. I have tested the correctness of mux settings printing out register values, and enabling/disabling the SCIF2 module connected to serial debug interface. The series makes use of newly introduced pin[ctrl|mux]_generic functions, currently only available in Linus Walleij's linux-pinctrl.git tree. I have merged the devel/ branch of that tree on top of Geert's renesas-drivers master one. The merge is available for clone at git://jmondi.org/linux in jmondi/renesas/pinctrl-devel branch, for the interested ones. Sending out as RFC, as I do expect lot of comments both from Renesas people that has a deeper knowledge of other RZ Series SoCs than me, and from gpio/pinctrl people as this is the first pin control driver I send out (so please bear with me on this :) Thank you j Jacopo Mondi (5): pinctrl: rz-pfc: Add Renesas RZ pinctrl core module pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver arm: dts: dt-bindings: Add Renesas RZ pinctrl header arm: dts: r7s1000: Add pincontroller node arm: dts: genmai: Add SCIF2 pin group arch/arm/boot/dts/r7s72100-genmai.dts | 13 + arch/arm/boot/dts/r7s72100.dtsi | 12 + drivers/pinctrl/Kconfig | 1 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/rz-pfc/Kconfig | 25 ++ drivers/pinctrl/rz-pfc/Makefile | 2 + drivers/pinctrl/rz-pfc/pinctrl-rz.c | 433 +++++++++++++++++++++++ drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 ++++++ drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 346 ++++++++++++++++++ include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 + 10 files changed, 966 insertions(+) create mode 100644 drivers/pinctrl/rz-pfc/Kconfig create mode 100644 drivers/pinctrl/rz-pfc/Makefile create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.c create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rza1.c create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h -- 2.7.4 ^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi @ 2017-01-25 18:09 ` Jacopo Mondi 2017-01-26 2:58 ` Chris Brandt ` (3 more replies) 2017-01-25 18:09 ` [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver Jacopo Mondi ` (7 subsequent siblings) 8 siblings, 4 replies; 54+ messages in thread From: Jacopo Mondi @ 2017-01-25 18:09 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Add core module for per-pin Renesas RZ series pin controller. The core module allows SoC driver to register their pins and SoC specific operations and interfaces with pinctrl and pinmux core on their behalf. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/pinctrl/Kconfig | 1 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/rz-pfc/Kconfig | 18 ++ drivers/pinctrl/rz-pfc/Makefile | 1 + drivers/pinctrl/rz-pfc/pinctrl-rz.c | 447 ++++++++++++++++++++++++++++++++++++ drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 +++++++++ 6 files changed, 582 insertions(+) create mode 100644 drivers/pinctrl/rz-pfc/Kconfig create mode 100644 drivers/pinctrl/rz-pfc/Makefile create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.c create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 8f8c2af..6d72e58 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -294,6 +294,7 @@ source "drivers/pinctrl/mvebu/Kconfig" source "drivers/pinctrl/nomadik/Kconfig" source "drivers/pinctrl/pxa/Kconfig" source "drivers/pinctrl/qcom/Kconfig" +source "drivers/pinctrl/rz-pfc/Kconfig" source "drivers/pinctrl/samsung/Kconfig" source "drivers/pinctrl/sh-pfc/Kconfig" source "drivers/pinctrl/spear/Kconfig" diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index a251f43..96e7ece 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -48,6 +48,7 @@ obj-$(CONFIG_PINCTRL_MVEBU) += mvebu/ obj-y += nomadik/ obj-$(CONFIG_PINCTRL_PXA) += pxa/ obj-$(CONFIG_ARCH_QCOM) += qcom/ +obj-$(CONFIG_PINCTRL_RZ_PINCTRL) += rz-pfc/ obj-$(CONFIG_PINCTRL_SAMSUNG) += samsung/ obj-$(CONFIG_PINCTRL_SH_PFC) += sh-pfc/ obj-$(CONFIG_PINCTRL_SPEAR) += spear/ diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig new file mode 100644 index 0000000..3714c10 --- /dev/null +++ b/drivers/pinctrl/rz-pfc/Kconfig @@ -0,0 +1,18 @@ +# +# Renesas RZ pinctrl drivers +# + +if ARCH_RENESAS + +config PINCTRL_RZ_PINCTRL + select PINMUX + select PINCONF + select GPIOLIB + select GENERIC_PINCONF + select GENERIC_PINMUX_FUNCTIONS + select GENERIC_PINCTRL_GROUPS + def_bool y + help + This enables pin control drivers for Renesas RZ platforms + +endif diff --git a/drivers/pinctrl/rz-pfc/Makefile b/drivers/pinctrl/rz-pfc/Makefile new file mode 100644 index 0000000..cba8283 --- /dev/null +++ b/drivers/pinctrl/rz-pfc/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_PINCTRL_RZ_PINCTRL) += pinctrl-rz.o diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rz.c b/drivers/pinctrl/rz-pfc/pinctrl-rz.c new file mode 100644 index 0000000..3efbf03 --- /dev/null +++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.c @@ -0,0 +1,447 @@ +/* + * Pinctrl support for Renesas RZ Series + * + * Copyright (C) 2017 Jacopo Mondi + * Copyright (C) 2017 Renesas Electronics Corporation + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/err.h> +#include <linux/init.h> +#include <linux/ioport.h> +#include <linux/module.h> + +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_device.h> + +#include <linux/pinctrl/pinconf-generic.h> +#include <linux/pinctrl/pinctrl.h> +#include <linux/pinctrl/pinmux.h> + +#include "../core.h" +#include "../devicetree.h" +#include "../pinmux.h" + +#include "pinctrl-rz.h" + +#define DRIVER_NAME "rz-pinctrl" +#define RZ_PIN_ARGS_COUNT 3 + +/** + * rz_pinctrl_pos_to_index() - Retrieve the index of pin at position [bank:pin] + * + * This can be improved, as it walks all the pins reported by the SoC driver + * + * @return: pin number between [0 - npins]; -1 if not found + */ +static int rz_pinctrl_pos_to_index(struct rz_pinctrl_dev *rz_pinctrl, + unsigned int bank, unsigned int pin) +{ + struct rz_pinctrl_info *info; + struct rz_pin_desc *rz_pin; + int i, npins; + + info = rz_pinctrl->info; + npins = info->npins; + for (i = 0; i < npins; ++i) { + rz_pin = &info->pins[i]; + + /* + * return the pin index in the array, not the pin number, + * so that we can access it easily when muxing group's pins + */ + if (rz_pin->bank == bank && rz_pin->pin == pin) + return i; + } + + return -1; +} + +/* ---------------------------------------------------------------------------- + * pinctrl operations + */ +static void rz_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s, + unsigned int pin) +{ + struct rz_pinctrl_dev *rz_pinctrl; + struct rz_pinctrl_info *info; + + rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); + info = rz_pinctrl->info; + + seq_printf(s, "%s %s\n", info->pins[pin].name, DRIVER_NAME); +} + +/** + * rz_dt_node_to_map() - Parse device tree nodes and collect pins, groups and + * functions + * + * Pins for RZ series pin controller described by "renesas-rz,pins" property + * are arrays of u32 values in the form: + * + * "renesas-rz,pins" = <BANK PIN MUX>, ... ; + * + * Parse the list arguments and identify pins through their position + * (bank and pin offset) and save the provided mux mode for later use. + * + * TODO: the array can be expended to support additional parameters for + * pin configurations values (IO direction etc) + * + * @pctldev: pin controller device + * @np_config: device tree node to parse + * @map: pointer to pin map (output) + * @num_maps: number of collected maps (output) + * + * @return: 0 for success; != 0 otherwise + */ +static int rz_dt_node_to_map(struct pinctrl_dev *pctldev, + struct device_node *np_config, + struct pinctrl_map **map, unsigned int *num_maps) +{ + struct rz_pinctrl_dev *rz_pinctrl; + const char *prop_name = "renesas-rz,pins", + *grpname, **fngrps; + unsigned int *grpins, + *mux_modes; + int i, npins, ret; + + rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); + + npins = pinctrl_count_index_with_args(np_config, prop_name); + if (npins <= 0) { + dev_err(rz_pinctrl->dev, "Missing pins configuration prop\n"); + return -EINVAL; + } + + mux_modes = devm_kzalloc(rz_pinctrl->dev, sizeof(*mux_modes) * npins, + GFP_KERNEL); + grpins = devm_kzalloc(rz_pinctrl->dev, sizeof(*grpins) * npins, + GFP_KERNEL); + if (unlikely(!grpins || !mux_modes)) + return -ENOMEM; + + /* + * functions are made of 1 group only; + * in facts, functions and groups are identical for RZ pin controller, + * except that functions carry an array of mux modes (aka alternate + * functions IDs) + */ + fngrps = devm_kzalloc(rz_pinctrl->dev, sizeof(*fngrps), GFP_KERNEL); + if (unlikely(!fngrps)) { + ret = -ENOMEM; + goto free_pins; + } + + *num_maps = 0; + (*map) = devm_kzalloc(rz_pinctrl->dev, sizeof(**map), GFP_KERNEL); + if (unlikely(!*map)) { + ret = -ENOMEM; + goto free_fngrps; + } + + /* collect pin numbers and mux confs to create groups and functions */ + for (i = 0; i < npins; ++i) { + struct of_phandle_args of_pins_args; + unsigned int bank, offs, mode, pin_idx; + + /* DTS identifies pin by position: bank and pin offset */ + ret = pinctrl_parse_index_with_args(np_config, prop_name, i, + &of_pins_args); + if (ret) + goto free_map; + + if (of_pins_args.args_count < RZ_PIN_ARGS_COUNT) { + dev_err(rz_pinctrl->dev, + "Wrong arguments number for renesas-rz,pins"); + ret = -EINVAL; + goto free_map; + } + + bank = of_pins_args.args[0]; + offs = of_pins_args.args[1]; + mode = of_pins_args.args[2]; + + /* pin_idx is the index on the static pin array */ + pin_idx = rz_pinctrl_pos_to_index(rz_pinctrl, bank, offs); + if (pin_idx < 0) { + dev_err(rz_pinctrl->dev, + "Invalid pin position %d-%d\n", bank, offs); + ret = -EINVAL; + goto free_map; + } + + grpins[i] = pin_idx; + mux_modes[i] = mode; + } + + grpname = np_config->name; + fngrps[0] = grpname; + + mutex_lock(&rz_pinctrl->mutex); + ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins, NULL); + if (ret) { + mutex_unlock(&rz_pinctrl->mutex); + goto free_map; + } + + ret = pinmux_generic_add_function(pctldev, grpname, fngrps, + 1, mux_modes); + if (ret) { + mutex_unlock(&rz_pinctrl->mutex); + goto free_map; + } + mutex_unlock(&rz_pinctrl->mutex); + + (*map)->type = PIN_MAP_TYPE_MUX_GROUP; + (*map)->data.mux.group = grpname; + (*map)->data.mux.function = grpname; + *num_maps = 1; + + return 0; + +free_map: + devm_kfree(rz_pinctrl->dev, *map); +free_fngrps: + devm_kfree(rz_pinctrl->dev, fngrps); +free_pins: + devm_kfree(rz_pinctrl->dev, mux_modes); + devm_kfree(rz_pinctrl->dev, grpins); + return ret; +} + +/** + * rz_dt_free_map() + */ +static void rz_dt_free_map(struct pinctrl_dev *pctldev, + struct pinctrl_map *map, unsigned int num_maps) +{ + struct rz_pinctrl_dev *rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); + + devm_kfree(rz_pinctrl->dev, map); +} + +static const struct pinctrl_ops rz_pinctrl_ops = { + .get_groups_count = pinctrl_generic_get_group_count, + .get_group_name = pinctrl_generic_get_group_name, + .get_group_pins = pinctrl_generic_get_group_pins, + .pin_dbg_show = rz_pin_dbg_show, + .dt_node_to_map = rz_dt_node_to_map, + .dt_free_map = rz_dt_free_map, +}; + +/* ---------------------------------------------------------------------------- + * pinconfig operations + */ + +/* TODO */ + +/* ---------------------------------------------------------------------------- + * pinmux operations + */ + +/** + * rz_pinmux_set() - Retrieve pins from a group and apply mux settings + * + * @pctldev: pin controller device + * @selector: Function selector + * @group: Group selector + * @return: 0 for success; != otherwise + */ +static int rz_pinmux_set(struct pinctrl_dev *pctldev, unsigned int selector, + unsigned int group) +{ + struct rz_pinctrl_dev *rz_pinctrl; + struct rz_pinctrl_info *info; + struct rz_pinctrl_ops *ops; + struct group_desc *grp; + struct function_desc *func; + unsigned int npins, *mux_modes; + int i; + + rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); + info = rz_pinctrl->info; + ops = info->ops; + + grp = pinctrl_generic_get_group(pctldev, group); + if (!grp) + return -EINVAL; + + func = pinmux_generic_get_function(pctldev, selector); + if (!func) + return -EINVAL; + + npins = grp->num_pins; + mux_modes = (unsigned int *)func->data; + + for (i = 0; i < npins; ++i) { + unsigned int pin_idx, mux_mode; + struct rz_pin_desc *pin; + int ret; + + /* + * use pin index to retrieve pin descriptor; + * then provide it to the set_mux SoC operation + */ + pin_idx = grp->pins[i]; + pin = &info->pins[pin_idx]; + mux_mode = mux_modes[i]; + + if (!ops || !ops->set_mux) { + dev_err(rz_pinctrl->dev, "Pin muxing not supported\n"); + return -EINVAL; + } + + ret = ops->set_mux(rz_pinctrl, pin, mux_mode); + if (ret) + return ret; + } + + return 0; +} + +struct pinmux_ops rz_pinmux_ops = { + .get_functions_count = pinmux_generic_get_function_count, + .get_function_name = pinmux_generic_get_function_name, + .get_function_groups = pinmux_generic_get_function_groups, + .set_mux = rz_pinmux_set, +}; + +/* ---------------------------------------------------------------------------- + * rz pinctrl operations + */ + +/** + * rz_add_pins() - Enumerate pins reported by SoC driver in pin desc array + * + * @rz_pinctrl: RZ pincontroller + * + * @return: 0 for success; != 0 otherwise + */ +static int rz_add_pins(struct rz_pinctrl_dev *rz_pinctrl) +{ + struct rz_pinctrl_info *info; + struct rz_pin_desc *pin; + unsigned int npins, i; + + info = rz_pinctrl->info; + npins = info->npins; + + rz_pinctrl->pins = devm_kzalloc(rz_pinctrl->dev, + npins * sizeof(*rz_pinctrl->pins), + GFP_KERNEL); + if (unlikely(!rz_pinctrl->pins)) + return -ENOMEM; + + rz_pinctrl->desc.pins = rz_pinctrl->pins; + rz_pinctrl->desc.npins = npins; + + for (i = 0; i < npins; ++i) { + pin = &info->pins[i]; + + rz_pinctrl->pins[i].number = pin->number; + rz_pinctrl->pins[i].name = pin->name; + } + + return 0; +} + +/** + * rz_pinctrl_map_res() - Map memory resources for pincontroller + * + * @pdev: platform device + * @rz_pincrl: RZ pincontroller + * + * @return: 0 for success; != 0 otherwise + */ +static int rz_pinctrl_map_res(struct platform_device *pdev, + struct rz_pinctrl_dev *rz_pinctrl) +{ + struct resource *res; + struct rz_pinctrl_res *rz_res; + unsigned int i, nres; + + nres = pdev->num_resources; + rz_res = devm_kzalloc(&pdev->dev, nres * sizeof(*rz_res), + GFP_KERNEL); + if (unlikely(!rz_res)) + return -ENOMEM; + + rz_pinctrl->nres = nres; + rz_pinctrl->res = rz_res; + + for (i = 0; i < nres; i++) { + res = platform_get_resource(pdev, IORESOURCE_MEM, i); + if (!res) + return -ENODEV; + + rz_res = &rz_pinctrl->res[i]; + + rz_res->start = res->start; + rz_res->size = resource_size(res); + rz_res->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(rz_res->base)) + return PTR_ERR(rz_res->base); + } + + return 0; +} + +/** + * rz_pinctrl_probe() - Register pincontroller driver operations + * + * @pdev: platform device + * @info: SoC device info structure + */ +int rz_pinctrl_probe(struct platform_device *pdev, struct rz_pinctrl_info *info) +{ + int ret; + struct rz_pinctrl_dev *rz_pinctrl; + + rz_pinctrl = devm_kzalloc(&pdev->dev, sizeof(*rz_pinctrl), GFP_KERNEL); + if (!rz_pinctrl) + return -ENOMEM; + + rz_pinctrl->dev = &pdev->dev; + rz_pinctrl->info = info; + + ret = rz_pinctrl_map_res(pdev, rz_pinctrl); + if (ret) + return ret; + + platform_set_drvdata(pdev, rz_pinctrl); + + rz_pinctrl->desc.name = DRIVER_NAME; + rz_pinctrl->desc.pctlops = &rz_pinctrl_ops; + rz_pinctrl->desc.pmxops = &rz_pinmux_ops; + rz_pinctrl->desc.owner = THIS_MODULE; + + ret = rz_add_pins(rz_pinctrl); + if (ret) + return ret; + + mutex_init(&rz_pinctrl->mutex); + + ret = pinctrl_register_and_init(&rz_pinctrl->desc, rz_pinctrl->dev, + rz_pinctrl, &rz_pinctrl->pctl); + if (ret) { + dev_err(&pdev->dev, "could not register rz pinctrl driver\n"); + return ret; + } + + return 0; +} + +void rz_pinctrl_remove(struct platform_device *pdev) +{ + struct rz_pinctrl_dev *rz_pinctrl = platform_get_drvdata(pdev); + + pinctrl_unregister(rz_pinctrl->pctl); +} + +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Pinctrl driver for Reneas RZ series"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rz.h b/drivers/pinctrl/rz-pfc/pinctrl-rz.h new file mode 100644 index 0000000..b873c89 --- /dev/null +++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.h @@ -0,0 +1,114 @@ +/* + * Pinctrl support for Renesas RZ Series + * + * Copyright (C) 2017 Jacopo Mondi + * Copyright (C) 2017 Renesas Electronics Corporation + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ +#ifndef __RZ_PINCTRL_H__ +#define __RZ_PINCTRL_H__ + +#include <linux/pinctrl/pinctrl.h> +#include <linux/types.h> + +/* ---------------------------------------------------------------------------- + * pinctrl-rz data types + */ +#define RZ_PIN_NAME(bank, pin) \ + PIN_##bank##_##pin + +#define RZ_PIN_DESC(b, p) \ + { .number = RZ_PIN_NAME(b, p), \ + .name = __stringify(RZ_PIN_NAME(b, p)), \ + .bank = b, .pin = p } + +/** + * rz_pin_desc - Single RZ pin descriptor + * + * @number: pin number as enumerated by SoC driver + * @name: pin name as reported by SoC driver + * @bank: pin bank location + * @pin: pin register offset + */ +struct rz_pin_desc { + unsigned int bank, pin; + unsigned int number; + const char *name; +}; + +/** + * rz_pinctrl_info - SoC info data + * + * @ops: SoC specific operations + * @npins: number of pins + * @pins: pin array + */ +struct rz_pinctrl_ops; +struct rz_pinctrl_info { + struct rz_pinctrl_ops *ops; + + unsigned int npins; + struct rz_pin_desc *pins; +}; + +/** + * rz_pinctrl_res - Memory resource for RZ SoC + * + * @start: physical address base + * @size: memory region size + * @base: logical address base + */ +struct rz_pinctrl_res { + resource_size_t start; + resource_size_t size; + void __iomem *base; +}; + +/** + * rz_pinctrl_dev - RZ pincontroller device + * + * @dev: device + * @info: SoC data + * @nres: number of memory regions + * @res: memory regions + * @mutex: protect pin*_generic functions + * @pins: pin array for pinctrl core + * @desc: pincontroller desc for pinctrl core + * @pctl: pinctrl device + */ +struct rz_pinctrl_dev { + struct device *dev; + + struct rz_pinctrl_info *info; + + unsigned int nres; + struct rz_pinctrl_res *res; + + struct mutex mutex; + + struct pinctrl_pin_desc *pins; + struct pinctrl_desc desc; + struct pinctrl_dev *pctl; +}; + +/** + * rz_pinctrl_ops - SoC operations + * + * @set_mux: perform pin muxing on SoC registers + */ +struct rz_pinctrl_ops { + int (*set_mux)(struct rz_pinctrl_dev *, struct rz_pin_desc *, + unsigned int mux_mode); +}; + +/* ---------------------------------------------------------------------------- + * pinctrl-rz prototypes + */ +int rz_pinctrl_probe(struct platform_device *pdev, + struct rz_pinctrl_info *info); +void rz_pinctrl_remove(struct platform_device *pdev); + +#endif -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* RE: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-25 18:09 ` [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module Jacopo Mondi @ 2017-01-26 2:58 ` Chris Brandt 2017-01-26 9:08 ` jacopo mondi 2017-01-26 19:43 ` Geert Uytterhoeven ` (2 subsequent siblings) 3 siblings, 1 reply; 54+ messages in thread From: Chris Brandt @ 2017-01-26 2:58 UTC (permalink / raw) To: Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hi Jacopo, On Wednesday, January 25, 2017, Jacopo Mondi wrote: > drivers/pinctrl/Kconfig | 1 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/rz-pfc/Kconfig | 18 ++ > drivers/pinctrl/rz-pfc/Makefile | 1 + > drivers/pinctrl/rz-pfc/pinctrl-rz.c | 447 > ++++++++++++++++++++++++++++++++++++ > drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 +++++++++ > 6 files changed, 582 insertions(+) > create mode 100644 drivers/pinctrl/rz-pfc/Kconfig create mode 100644 > drivers/pinctrl/rz-pfc/Makefile create mode 100644 drivers/pinctrl/rz- > pfc/pinctrl-rz.c > create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h I think we should try to avoid the rz naming as much as possible since this driver will hopefully be useful for other future Renesas devices if they move to a similar pin-control type method. Maybe future "R-car" SoCs? Or, maybe Renesas Marketing decides to come up with a new name for SoCs. Otherwise, you could end up with a directly like "mach-shmobile" filled with a bunch of...well...not SH-Mobile parts. Maybe just drivers/pinctrl/renesas/pinctrl-renesas.c Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-26 2:58 ` Chris Brandt @ 2017-01-26 9:08 ` jacopo mondi 2017-01-26 14:19 ` Chris Brandt 0 siblings, 1 reply; 54+ messages in thread From: jacopo mondi @ 2017-01-26 9:08 UTC (permalink / raw) To: Chris Brandt, Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hi Chris, On 26/01/2017 03:58, Chris Brandt wrote: > Hi Jacopo, > > On Wednesday, January 25, 2017, Jacopo Mondi wrote: >> drivers/pinctrl/Kconfig | 1 + >> drivers/pinctrl/Makefile | 1 + >> drivers/pinctrl/rz-pfc/Kconfig | 18 ++ >> drivers/pinctrl/rz-pfc/Makefile | 1 + >> drivers/pinctrl/rz-pfc/pinctrl-rz.c | 447 >> ++++++++++++++++++++++++++++++++++++ >> drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 +++++++++ >> 6 files changed, 582 insertions(+) >> create mode 100644 drivers/pinctrl/rz-pfc/Kconfig create mode 100644 >> drivers/pinctrl/rz-pfc/Makefile create mode 100644 drivers/pinctrl/rz- >> pfc/pinctrl-rz.c >> create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h > > > > I think we should try to avoid the rz naming as much as possible since > this driver will hopefully be useful for other future Renesas devices if > they move to a similar pin-control type method. Maybe future "R-car" SoCs? > Or, maybe Renesas Marketing decides to come up with a new name for SoCs. > > Otherwise, you could end up with a directly like "mach-shmobile" filled > with a bunch of...well...not SH-Mobile parts. > > Maybe just > drivers/pinctrl/renesas/pinctrl-renesas.c > Wouldn't this be confusing, as most of renesas SoC are supported through drivers/pinctrl/sh-pfc instead? I agree on dropping the rz name, if more SoC will come and join the pin-based PFC realm... Thanks j > > > Chris > > -- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-26 9:08 ` jacopo mondi @ 2017-01-26 14:19 ` Chris Brandt 0 siblings, 0 replies; 54+ messages in thread From: Chris Brandt @ 2017-01-26 14:19 UTC (permalink / raw) To: jacopo mondi, Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hi Jacopo, On Thursday, January 26, 2017, jacopo mondi wrote: > > I think we should try to avoid the rz naming as much as possible since > > this driver will hopefully be useful for other future Renesas devices > > if they move to a similar pin-control type method. Maybe future "R-car" > SoCs? > > Or, maybe Renesas Marketing decides to come up with a new name for SoCs. > > > > Otherwise, you could end up with a directly like "mach-shmobile" > > filled with a bunch of...well...not SH-Mobile parts. > > > > Maybe just > > drivers/pinctrl/renesas/pinctrl-renesas.c > > > > Wouldn't this be confusing, as most of renesas SoC are supported through > drivers/pinctrl/sh-pfc instead? It's already confusing that a bunch of ARM parts use a "sh" pin controller. In reality, any Renesas device could be a peripheral mix of ex-Mitsubishi, ex-Hitachi, ex-NEC or Renesas original. Or, some IP was licensed from a 3rd party (ie, the SDHI/TMIO controller). The only thing consistent is "Renesas". > I agree on dropping the rz name, if more SoC will come and join the pin- > based PFC realm... I will suggest it for the next generation of SoCs, so we'll see what happens... Thanks, Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-25 18:09 ` [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module Jacopo Mondi 2017-01-26 2:58 ` Chris Brandt @ 2017-01-26 19:43 ` Geert Uytterhoeven 2017-01-30 19:19 ` Chris Brandt 2017-02-01 15:21 ` Laurent Pinchart 3 siblings, 0 replies; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-26 19:43 UTC (permalink / raw) To: Jacopo Mondi Cc: Laurent Pinchart, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Jacopo, On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > Add core module for per-pin Renesas RZ series pin controller. > The core module allows SoC driver to register their pins and SoC > specific operations and interfaces with pinctrl and pinmux core on their > behalf. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> I believe nothing besides the DT property "renesas-rz,pins" and the value of the RZ_PIN_ARGS_COUNT macro is really specific to Renesas or RZ(A)? So this could become something really generic, and part of core pinctrl? > --- /dev/null > +++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.c > @@ -0,0 +1,447 @@ > +#define RZ_PIN_ARGS_COUNT 3 This should be a parameter in the SoC-specific subdriver. > +/** > + * rz_pinctrl_pos_to_index() - Retrieve the index of pin at position [bank:pin] > + * > + * This can be improved, as it walks all the pins reported by the SoC driver > + * > + * @return: pin number between [0 - npins]; -1 if not found > + */ > +static int rz_pinctrl_pos_to_index(struct rz_pinctrl_dev *rz_pinctrl, > + unsigned int bank, unsigned int pin) > +{ > + struct rz_pinctrl_info *info; > + struct rz_pin_desc *rz_pin; const > + int i, npins; unsigned int > +/* ---------------------------------------------------------------------------- > + * pinctrl operations > + */ > +static void rz_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s, const ... pctldev > + unsigned int pin) > +{ > + struct rz_pinctrl_dev *rz_pinctrl; > + struct rz_pinctrl_info *info; const > +static int rz_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np_config, > + struct pinctrl_map **map, unsigned int *num_maps) > +{ > + fngrps = devm_kzalloc(rz_pinctrl->dev, sizeof(*fngrps), GFP_KERNEL); > + if (unlikely(!fngrps)) { Please don't use unlikely. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-25 18:09 ` [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module Jacopo Mondi 2017-01-26 2:58 ` Chris Brandt 2017-01-26 19:43 ` Geert Uytterhoeven @ 2017-01-30 19:19 ` Chris Brandt 2017-01-31 9:00 ` jacopo mondi 2017-02-01 15:21 ` Laurent Pinchart 3 siblings, 1 reply; 54+ messages in thread From: Chris Brandt @ 2017-01-30 19:19 UTC (permalink / raw) To: Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hi Jacopo, On Wednesday, January 25, 2017, Jacopo Mondi wrote: > + > + return 0; > + > +free_map: > + devm_kfree(rz_pinctrl->dev, *map); > +free_fngrps: > + devm_kfree(rz_pinctrl->dev, fngrps); > +free_pins: > + devm_kfree(rz_pinctrl->dev, mux_modes); > + devm_kfree(rz_pinctrl->dev, grpins); > + return ret; > +} Since one of the benefits of using devm_kzalloc is that if the probe fails and returns an error, all the memory associated with that device will automatically get freed, you 'might' not need this code to free memory. I say might because I'm not sure if returning an error here will kill the driver or not. But, might be interesting to look into. > +#define RZ_PIN_NAME(bank, pin) \ > + PIN_##bank##_##pin > + > +#define RZ_PIN_DESC(b, p) \ > + { .number = RZ_PIN_NAME(b, p), \ > + .name = __stringify(RZ_PIN_NAME(b, p)), \ The hardware manual uses the names "P1_0" for ports, so it might be better to match that format for consistency. Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-30 19:19 ` Chris Brandt @ 2017-01-31 9:00 ` jacopo mondi 2017-01-31 13:48 ` Chris Brandt 0 siblings, 1 reply; 54+ messages in thread From: jacopo mondi @ 2017-01-31 9:00 UTC (permalink / raw) To: Chris Brandt, Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hi Chris, On 30/01/2017 20:19, Chris Brandt wrote: > Hi Jacopo, > > On Wednesday, January 25, 2017, Jacopo Mondi wrote: >> + >> + return 0; >> + >> +free_map: >> + devm_kfree(rz_pinctrl->dev, *map); >> +free_fngrps: >> + devm_kfree(rz_pinctrl->dev, fngrps); >> +free_pins: >> + devm_kfree(rz_pinctrl->dev, mux_modes); >> + devm_kfree(rz_pinctrl->dev, grpins); >> + return ret; >> +} > > Since one of the benefits of using devm_kzalloc is that if > the probe fails and returns an error, all the memory associated with > that device will automatically get freed, you 'might' not > need this code to free memory. > > I say might because I'm not sure if returning an error here will > kill the driver or not. But, might be interesting to look into. > No, returning an error here would not kill the driver BUT if dt_node_to_map fails, dt_free_map is called immediately later [1] So here we maybe should not use device managed memory as it does not bring anything, do not free *map as it is freed later in dt_free_map, but release fngrps mux_modes and grpins, as we lose reference to them outside the scope of this function. Do you agree? > >> +#define RZ_PIN_NAME(bank, pin) \ >> + PIN_##bank##_##pin >> + >> +#define RZ_PIN_DESC(b, p) \ >> + { .number = RZ_PIN_NAME(b, p), \ >> + .name = __stringify(RZ_PIN_NAME(b, p)), \ > > The hardware manual uses the names "P1_0" for ports, so it might > be better to match that format for consistency. > > Noted Thanks j [1] http://lxr.free-electrons.com/source/drivers/pinctrl/devicetree.c#L236 ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-31 9:00 ` jacopo mondi @ 2017-01-31 13:48 ` Chris Brandt 2017-02-01 12:47 ` Laurent Pinchart 0 siblings, 1 reply; 54+ messages in thread From: Chris Brandt @ 2017-01-31 13:48 UTC (permalink / raw) To: jacopo mondi, Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hi Jacopo, On Tuesday, January 31, 2017, Jacopo Mondi wrote: > > Since one of the benefits of using devm_kzalloc is that if the probe > > fails and returns an error, all the memory associated with that device > > will automatically get freed, you 'might' not need this code to free > > memory. > > > > I say might because I'm not sure if returning an error here will kill > > the driver or not. But, might be interesting to look into. > > > > No, returning an error here would not kill the driver BUT if > dt_node_to_map fails, dt_free_map is called immediately later [1] > > So here we maybe should not use device managed memory as it does not bring > anything, do not free *map as it is freed later in dt_free_map, but > release fngrps mux_modes and grpins, as we lose reference to them outside > the scope of this function. > > Do you agree? Like you said, there is no benefit one way of the other. But, doing a grep of the pinctrl directory, devm_kzalloc is used more by the other drivers than kzalloc, so maybe we just stick with devm_kzalloc (baah goes the sheep) Chris > -----Original Message----- > From: jacopo mondi [mailto:jacopo@jmondi.org] > Sent: Tuesday, January 31, 2017 4:01 AM > To: Chris Brandt <Chris.Brandt@renesas.com>; Jacopo Mondi > <jacopo+renesas@jmondi.org>; laurent.pinchart@ideasonboard.com; > geert+renesas@glider.be; linus.walleij@linaro.org > Cc: linux-renesas-soc@vger.kernel.org; linux-gpio@vger.kernel.org > Subject: Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module > > Hi Chris, > > On 30/01/2017 20:19, Chris Brandt wrote: > > Hi Jacopo, > > > > On Wednesday, January 25, 2017, Jacopo Mondi wrote: > >> + > >> + return 0; > >> + > >> +free_map: > >> + devm_kfree(rz_pinctrl->dev, *map); > >> +free_fngrps: > >> + devm_kfree(rz_pinctrl->dev, fngrps); > >> +free_pins: > >> + devm_kfree(rz_pinctrl->dev, mux_modes); > >> + devm_kfree(rz_pinctrl->dev, grpins); > >> + return ret; > >> +} > > > > Since one of the benefits of using devm_kzalloc is that if the probe > > fails and returns an error, all the memory associated with that device > > will automatically get freed, you 'might' not need this code to free > > memory. > > > > I say might because I'm not sure if returning an error here will kill > > the driver or not. But, might be interesting to look into. > > > > No, returning an error here would not kill the driver BUT if > dt_node_to_map fails, dt_free_map is called immediately later [1] > > So here we maybe should not use device managed memory as it does not bring > anything, do not free *map as it is freed later in dt_free_map, but > release fngrps mux_modes and grpins, as we lose reference to them outside > the scope of this function. > > Do you agree? > > > > > >> +#define RZ_PIN_NAME(bank, pin) \ > >> + PIN_##bank##_##pin > >> + > >> +#define RZ_PIN_DESC(b, p) \ > >> + { .number = RZ_PIN_NAME(b, p), \ > >> + .name = __stringify(RZ_PIN_NAME(b, p)), \ > > > > The hardware manual uses the names "P1_0" for ports, so it might be > > better to match that format for consistency. > > > > > > Noted > > Thanks > j > > > [1] http://lxr.free-electrons.com/source/drivers/pinctrl/devicetree.c#L236 > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-31 13:48 ` Chris Brandt @ 2017-02-01 12:47 ` Laurent Pinchart 0 siblings, 0 replies; 54+ messages in thread From: Laurent Pinchart @ 2017-02-01 12:47 UTC (permalink / raw) To: Chris Brandt Cc: jacopo mondi, Jacopo Mondi, geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Chris, On Tuesday 31 Jan 2017 13:48:57 Chris Brandt wrote: > On Tuesday, January 31, 2017, Jacopo Mondi wrote: > >> Since one of the benefits of using devm_kzalloc is that if the probe > >> fails and returns an error, all the memory associated with that device > >> will automatically get freed, you 'might' not need this code to free > >> memory. > >> > >> I say might because I'm not sure if returning an error here will kill > >> the driver or not. But, might be interesting to look into. > > > > No, returning an error here would not kill the driver BUT if > > dt_node_to_map fails, dt_free_map is called immediately later [1] > > > > So here we maybe should not use device managed memory as it does not bring > > anything, do not free *map as it is freed later in dt_free_map, but > > release fngrps mux_modes and grpins, as we lose reference to them outside > > the scope of this function. > > > > Do you agree? > > Like you said, there is no benefit one way of the other. > > But, doing a grep of the pinctrl directory, devm_kzalloc is used more by > the other drivers than kzalloc, so maybe we just stick with devm_kzalloc > > (baah goes the sheep) Given that the devm_*() functions incur an overhead, let's use kzalloc/kfree directly. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-01-25 18:09 ` [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module Jacopo Mondi ` (2 preceding siblings ...) 2017-01-30 19:19 ` Chris Brandt @ 2017-02-01 15:21 ` Laurent Pinchart 2017-02-06 18:15 ` jacopo mondi 3 siblings, 1 reply; 54+ messages in thread From: Laurent Pinchart @ 2017-02-01 15:21 UTC (permalink / raw) To: Jacopo Mondi; +Cc: geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Jacopo, Thank you for the patch. On Wednesday 25 Jan 2017 19:09:43 Jacopo Mondi wrote: > Add core module for per-pin Renesas RZ series pin controller. > The core module allows SoC driver to register their pins and SoC > specific operations and interfaces with pinctrl and pinmux core on their > behalf. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/pinctrl/Kconfig | 1 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/rz-pfc/Kconfig | 18 ++ > drivers/pinctrl/rz-pfc/Makefile | 1 + > drivers/pinctrl/rz-pfc/pinctrl-rz.c | 447 ++++++++++++++++++++++++++++++++ > drivers/pinctrl/rz-pfc/pinctrl-rz.h | 114 +++++++++ > 6 files changed, 582 insertions(+) > create mode 100644 drivers/pinctrl/rz-pfc/Kconfig > create mode 100644 drivers/pinctrl/rz-pfc/Makefile > create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.c > create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rz.h As Chris pointed out, s/rz-pfc/renesas/ would be a good idea. This code isn't specific to RZ chips (even if it's only used by them at the moment), so the rz prefix and suffix should probably replaced with something else (in file names and in the code too). [snip] > diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig > new file mode 100644 > index 0000000..3714c10 > --- /dev/null > +++ b/drivers/pinctrl/rz-pfc/Kconfig > @@ -0,0 +1,18 @@ > +# > +# Renesas RZ pinctrl drivers > +# > + > +if ARCH_RENESAS > + > +config PINCTRL_RZ_PINCTRL You can add a depends on ARCH_RENESAS to replace the above if. It should actually be depends on ARCH_RENESAS || COMPILE_TEST to extend compile-test coverage. An even better option could be to drop the depends line completely, and replace def_bool with bool. That way the option will only be enabled if explicitly selected by another option (as done in patch 2/5). > + select PINMUX > + select PINCONF > + select GPIOLIB > + select GENERIC_PINCONF > + select GENERIC_PINMUX_FUNCTIONS > + select GENERIC_PINCTRL_GROUPS > + def_bool y Won't this enable the driver by default on all platforms, while only RZ needs it ? It's also customary to have the bool/tristate line as the very first line in the config option. > + help > + This enables pin control drivers for Renesas RZ platforms > + > +endif [snip] > diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rz.c > b/drivers/pinctrl/rz-pfc/pinctrl-rz.c new file mode 100644 > index 0000000..3efbf03 > --- /dev/null > +++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.c > @@ -0,0 +1,447 @@ > +/* > + * Pinctrl support for Renesas RZ Series > + * > + * Copyright (C) 2017 Jacopo Mondi > + * Copyright (C) 2017 Renesas Electronics Corporation > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/ioport.h> > +#include <linux/module.h> > + No need for a blank line here. > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > + Or here. > +#include <linux/pinctrl/pinconf-generic.h> > +#include <linux/pinctrl/pinctrl.h> > +#include <linux/pinctrl/pinmux.h> > + > +#include "../core.h" > +#include "../devicetree.h" > +#include "../pinmux.h" > + > +#include "pinctrl-rz.h" > + > +#define DRIVER_NAME "rz-pinctrl" > +#define RZ_PIN_ARGS_COUNT 3 > + > +/** > + * rz_pinctrl_pos_to_index() - Retrieve the index of pin at position > [bank:pin] > + * > + * This can be improved, as it walks all the pins reported by the SoC > driver > + * > + * @return: pin number between [0 - npins]; -1 if not found > + */ > +static int rz_pinctrl_pos_to_index(struct rz_pinctrl_dev *rz_pinctrl, > + unsigned int bank, unsigned int pin) > +{ > + struct rz_pinctrl_info *info; > + struct rz_pin_desc *rz_pin; > + int i, npins; i and npins are never negative, you can make them unsigned int. One variable declaration per line is also preferred. > + > + info = rz_pinctrl->info; You can move this to the variable declaration line. > + npins = info->npins; > + for (i = 0; i < npins; ++i) { You can drop the npins variable and use info->npins directly. > + rz_pin = &info->pins[i]; > + > + /* > + * return the pin index in the array, not the pin number, > + * so that we can access it easily when muxing group's pins Please start sentences with a capital letter and end them with a period. > + */ > + if (rz_pin->bank == bank && rz_pin->pin == pin) > + return i; > + } > + > + return -1; How about -EINVAL ? -1 is -EPERM, which would not be an appropriate error code if you end up leaking it to upper layers. > +} > + > +/* ------------------------------------------------------------------------ > + * pinctrl operations > + */ > +static void rz_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file > *s, > + unsigned int pin) > +{ > + struct rz_pinctrl_dev *rz_pinctrl; > + struct rz_pinctrl_info *info; > + > + rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); > + info = rz_pinctrl->info; You can also assign at declaration time here. > + seq_printf(s, "%s %s\n", info->pins[pin].name, DRIVER_NAME); The pinctrl core already prints the pin name: seq_printf(s, "pin %d (%s) ", pin, desc->name); /* Driver-specific info per pin */ if (ops->pin_dbg_show) ops->pin_dbg_show(pctldev, s, pin); You can just print the driver name here. I'm actually wondering whether printing the driver name is useful, or if we could simply drop this function. > +} > + > +/** > + * rz_dt_node_to_map() - Parse device tree nodes and collect pins, groups > and > + * functions I don't think we have groups and functions, do we ? > + * > + * Pins for RZ series pin controller described by "renesas-rz,pins" > property > + * are arrays of u32 values in the form: As mentioned elsewhere, "renesas,pins" will do. We could even use "pins", but then we'd have to comply with the pinctrl generic bindings (with a bin number in the first call instead of separate bank and pin for instance). > + * > + * "renesas-rz,pins" = <BANK PIN MUX>, ... ; > + * > + * Parse the list arguments and identify pins through their position > + * (bank and pin offset) and save the provided mux mode for later use. > + * > + * TODO: the array can be expended to support additional parameters for > + * pin configurations values (IO direction etc) > + * > + * @pctldev: pin controller device > + * @np_config: device tree node to parse > + * @map: pointer to pin map (output) > + * @num_maps: number of collected maps (output) > + * > + * @return: 0 for success; != 0 otherwise "; a negative error code otherwise" ? The function never returns a positive value. I don't think kerneldoc uses @return. Please try to compile the documentation and fix errors and warnings. > + */ > +static int rz_dt_node_to_map(struct pinctrl_dev *pctldev, > + struct device_node *np_config, > + struct pinctrl_map **map, unsigned int *num_maps) > +{ > + struct rz_pinctrl_dev *rz_pinctrl; > + const char *prop_name = "renesas-rz,pins", > + *grpname, **fngrps; > + unsigned int *grpins, > + *mux_modes; One variable declaration per line please. > + int i, npins, ret; i is never negative, you can make it an unsigned int. > + > + rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); You can initialize the variable at declaration time. > + npins = pinctrl_count_index_with_args(np_config, prop_name); > + if (npins <= 0) { > + dev_err(rz_pinctrl->dev, "Missing pins configuration prop\n"); Maybe (... "Missing %s property\n", prop_name); to be more explicit ? > + return -EINVAL; > + } > + > + mux_modes = devm_kzalloc(rz_pinctrl->dev, sizeof(*mux_modes) * npins, > + GFP_KERNEL); > + grpins = devm_kzalloc(rz_pinctrl->dev, sizeof(*grpins) * npins, > + GFP_KERNEL); As explained in another e-mail, you can use kzalloc() here instead of devm_kzalloc(). Or, even better, kcalloc(). > + if (unlikely(!grpins || !mux_modes)) > + return -ENOMEM; > + > + /* > + * functions are made of 1 group only; > + * in facts, functions and groups are identical for RZ pin controller, > + * except that functions carry an array of mux modes (aka alternate > + * functions IDs) > + */ > + fngrps = devm_kzalloc(rz_pinctrl->dev, sizeof(*fngrps), GFP_KERNEL); > + if (unlikely(!fngrps)) { > + ret = -ENOMEM; > + goto free_pins; > + } > + > + *num_maps = 0; > + (*map) = devm_kzalloc(rz_pinctrl->dev, sizeof(**map), GFP_KERNEL); No need for parentheses around *map. > + if (unlikely(!*map)) { > + ret = -ENOMEM; > + goto free_fngrps; > + } > + > + /* collect pin numbers and mux confs to create groups and functions */ > + for (i = 0; i < npins; ++i) { > + struct of_phandle_args of_pins_args; > + unsigned int bank, offs, mode, pin_idx; You can spell offset fully. > + > + /* DTS identifies pin by position: bank and pin offset */ > + ret = pinctrl_parse_index_with_args(np_config, prop_name, i, > + &of_pins_args); > + if (ret) > + goto free_map; > + > + if (of_pins_args.args_count < RZ_PIN_ARGS_COUNT) { > + dev_err(rz_pinctrl->dev, > + "Wrong arguments number for renesas-rz,pins"); > + ret = -EINVAL; > + goto free_map; > + } > + > + bank = of_pins_args.args[0]; > + offs = of_pins_args.args[1]; > + mode = of_pins_args.args[2]; > + > + /* pin_idx is the index on the static pin array */ > + pin_idx = rz_pinctrl_pos_to_index(rz_pinctrl, bank, offs); > + if (pin_idx < 0) { > + dev_err(rz_pinctrl->dev, > + "Invalid pin position %d-%d\n", bank, offs); bank and offset are unsigned, you should use %u. > + ret = -EINVAL; > + goto free_map; > + } > + > + grpins[i] = pin_idx; > + mux_modes[i] = mode; > + } > + > + grpname = np_config->name; > + fngrps[0] = grpname; > + > + mutex_lock(&rz_pinctrl->mutex); This seems weird. The pinctrl generic functions indeed state that the caller must take care of locking, but there doesn't seem to be any lock protecting races between .dt_node_to_map() calling the generic functions below, and the generic .get_group_*() handlers. > + ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins, NULL); > + if (ret) { > + mutex_unlock(&rz_pinctrl->mutex); > + goto free_map; > + } > + > + ret = pinmux_generic_add_function(pctldev, grpname, fngrps, > + 1, mux_modes); > + if (ret) { > + mutex_unlock(&rz_pinctrl->mutex); > + goto free_map; > + } > + mutex_unlock(&rz_pinctrl->mutex); > + > + (*map)->type = PIN_MAP_TYPE_MUX_GROUP; > + (*map)->data.mux.group = grpname; > + (*map)->data.mux.function = grpname; > + *num_maps = 1; > + > + return 0; > + > +free_map: > + devm_kfree(rz_pinctrl->dev, *map); > +free_fngrps: > + devm_kfree(rz_pinctrl->dev, fngrps); > +free_pins: > + devm_kfree(rz_pinctrl->dev, mux_modes); > + devm_kfree(rz_pinctrl->dev, grpins); > + return ret; > +} > + > +/** > + * rz_dt_free_map() > + */ > +static void rz_dt_free_map(struct pinctrl_dev *pctldev, > + struct pinctrl_map *map, unsigned int num_maps) > +{ > + struct rz_pinctrl_dev *rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); > + > + devm_kfree(rz_pinctrl->dev, map); Shouldn't you also free the other allocated pieces of memory (fngrps, mux_modes and grpins) ? > +} > + > +static const struct pinctrl_ops rz_pinctrl_ops = { > + .get_groups_count = pinctrl_generic_get_group_count, > + .get_group_name = pinctrl_generic_get_group_name, > + .get_group_pins = pinctrl_generic_get_group_pins, > + .pin_dbg_show = rz_pin_dbg_show, > + .dt_node_to_map = rz_dt_node_to_map, > + .dt_free_map = rz_dt_free_map, > +}; > + > +/* ------------------------------------------------------------------------ > + * pinconfig operations > + */ > + > +/* TODO */ Yes ? :-) > +/* ------------------------------------------------------------------------ > + * pinmux operations > + */ > + > +/** > + * rz_pinmux_set() - Retrieve pins from a group and apply mux settings > + * > + * @pctldev: pin controller device > + * @selector: Function selector > + * @group: Group selector > + * @return: 0 for success; != otherwise Same comment as above. > + */ > +static int rz_pinmux_set(struct pinctrl_dev *pctldev, unsigned int > selector, > + unsigned int group) > +{ > + struct rz_pinctrl_dev *rz_pinctrl; > + struct rz_pinctrl_info *info; > + struct rz_pinctrl_ops *ops; > + struct group_desc *grp; > + struct function_desc *func; > + unsigned int npins, *mux_modes; > + int i; i is never negative. > + > + rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); > + info = rz_pinctrl->info; > + ops = info->ops; You can do this at variable declaration time. > + grp = pinctrl_generic_get_group(pctldev, group); > + if (!grp) > + return -EINVAL; > + > + func = pinmux_generic_get_function(pctldev, selector); > + if (!func) > + return -EINVAL; > + > + npins = grp->num_pins; No need for an intermediate variable, use grp->num_pins directly. > + mux_modes = (unsigned int *)func->data; > + > + for (i = 0; i < npins; ++i) { > + unsigned int pin_idx, mux_mode; > + struct rz_pin_desc *pin; > + int ret; > + > + /* > + * use pin index to retrieve pin descriptor; > + * then provide it to the set_mux SoC operation > + */ > + pin_idx = grp->pins[i]; > + pin = &info->pins[pin_idx]; > + mux_mode = mux_modes[i]; > + > + if (!ops || !ops->set_mux) { > + dev_err(rz_pinctrl->dev, "Pin muxing not supported\n"); > + return -EINVAL; > + } > + > + ret = ops->set_mux(rz_pinctrl, pin, mux_mode); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +struct pinmux_ops rz_pinmux_ops = { > + .get_functions_count = pinmux_generic_get_function_count, > + .get_function_name = pinmux_generic_get_function_name, > + .get_function_groups = pinmux_generic_get_function_groups, > + .set_mux = rz_pinmux_set, > +}; > + > +/* ------------------------------------------------------------------------ > + * rz pinctrl operations > + */ > + > +/** > + * rz_add_pins() - Enumerate pins reported by SoC driver in pin desc array Does this function really enumerate pins ? > + * > + * @rz_pinctrl: RZ pincontroller > + * > + * @return: 0 for success; != 0 otherwise Same as above, and some comment for below. > + */ > +static int rz_add_pins(struct rz_pinctrl_dev *rz_pinctrl) > +{ > + struct rz_pinctrl_info *info; > + struct rz_pin_desc *pin; > + unsigned int npins, i; > + > + info = rz_pinctrl->info; > + npins = info->npins; Fold this with variable declaration. I'd get rid of the npins variable, you can use info->npins. > + rz_pinctrl->pins = devm_kzalloc(rz_pinctrl->dev, > + npins * sizeof(*rz_pinctrl->pins), > + GFP_KERNEL); kcalloc. > + if (unlikely(!rz_pinctrl->pins)) > + return -ENOMEM; > + > + rz_pinctrl->desc.pins = rz_pinctrl->pins; > + rz_pinctrl->desc.npins = npins; > + > + for (i = 0; i < npins; ++i) { > + pin = &info->pins[i]; > + > + rz_pinctrl->pins[i].number = pin->number; > + rz_pinctrl->pins[i].name = pin->name; > + } > + > + return 0; > +} > + > +/** > + * rz_pinctrl_map_res() - Map memory resources for pincontroller You can call the function ..._map_resources. > + * @pdev: platform device > + * @rz_pincrl: RZ pincontroller > + * > + * @return: 0 for success; != 0 otherwise > + */ > +static int rz_pinctrl_map_res(struct platform_device *pdev, > + struct rz_pinctrl_dev *rz_pinctrl) > +{ > + struct resource *res; > + struct rz_pinctrl_res *rz_res; > + unsigned int i, nres; > + > + nres = pdev->num_resources; > + rz_res = devm_kzalloc(&pdev->dev, nres * sizeof(*rz_res), > + GFP_KERNEL); kcalloc() here too. > + if (unlikely(!rz_res)) > + return -ENOMEM; > + > + rz_pinctrl->nres = nres; > + rz_pinctrl->res = rz_res; > + > + for (i = 0; i < nres; i++) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, i); > + if (!res) > + return -ENODEV; > + > + rz_res = &rz_pinctrl->res[i]; > + > + rz_res->start = res->start; > + rz_res->size = resource_size(res); If you remove the debugging statement from patch 2/5 you can drop the start and size fields. > + rz_res->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(rz_res->base)) > + return PTR_ERR(rz_res->base); > + } > + > + return 0; > +} > + > +/** > + * rz_pinctrl_probe() - Register pincontroller driver operations > + * > + * @pdev: platform device > + * @info: SoC device info structure > + */ > +int rz_pinctrl_probe(struct platform_device *pdev, struct rz_pinctrl_info > *info) > +{ > + int ret; > + struct rz_pinctrl_dev *rz_pinctrl; > + > + rz_pinctrl = devm_kzalloc(&pdev->dev, sizeof(*rz_pinctrl), GFP_KERNEL); kzalloc() > + if (!rz_pinctrl) > + return -ENOMEM; > + > + rz_pinctrl->dev = &pdev->dev; > + rz_pinctrl->info = info; > + > + ret = rz_pinctrl_map_res(pdev, rz_pinctrl); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, rz_pinctrl); > + > + rz_pinctrl->desc.name = DRIVER_NAME; > + rz_pinctrl->desc.pctlops = &rz_pinctrl_ops; > + rz_pinctrl->desc.pmxops = &rz_pinmux_ops; > + rz_pinctrl->desc.owner = THIS_MODULE; > + > + ret = rz_add_pins(rz_pinctrl); > + if (ret) > + return ret; > + > + mutex_init(&rz_pinctrl->mutex); I'd move this earlier, to make sure the mutex is initialized before it gets used in case we start using it in rz_pinctrl_map_res() or rz_add_pins() later. > + ret = pinctrl_register_and_init(&rz_pinctrl->desc, rz_pinctrl->dev, > + rz_pinctrl, &rz_pinctrl->pctl); > + if (ret) { > + dev_err(&pdev->dev, "could not register rz pinctrl driver\n"); > + return ret; > + } > + > + return 0; > +} > + > +void rz_pinctrl_remove(struct platform_device *pdev) > +{ > + struct rz_pinctrl_dev *rz_pinctrl = platform_get_drvdata(pdev); > + > + pinctrl_unregister(rz_pinctrl->pctl); > +} The code is always compiled in, and the device never goes away. I'd drop remove support, as it's completely useless. You then need to set the device_driver structure suppress_bind_attrs field. > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org"); > +MODULE_DESCRIPTION("Pinctrl driver for Reneas RZ series"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rz.h > b/drivers/pinctrl/rz-pfc/pinctrl-rz.h new file mode 100644 > index 0000000..b873c89 > --- /dev/null > +++ b/drivers/pinctrl/rz-pfc/pinctrl-rz.h > @@ -0,0 +1,114 @@ > +/* > + * Pinctrl support for Renesas RZ Series > + * > + * Copyright (C) 2017 Jacopo Mondi > + * Copyright (C) 2017 Renesas Electronics Corporation > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > +#ifndef __RZ_PINCTRL_H__ > +#define __RZ_PINCTRL_H__ > + > +#include <linux/pinctrl/pinctrl.h> > +#include <linux/types.h> > + > +/* > --------------------------------------------------------------------------- > - + * pinctrl-rz data types > + */ > +#define RZ_PIN_NAME(bank, pin) \ > + PIN_##bank##_##pin > + > +#define RZ_PIN_DESC(b, p) \ > + { .number = RZ_PIN_NAME(b, p), \ > + .name = __stringify(RZ_PIN_NAME(b, p)), \ > + .bank = b, .pin = p } > + > +/** > + * rz_pin_desc - Single RZ pin descriptor > + * > + * @number: pin number as enumerated by SoC driver > + * @name: pin name as reported by SoC driver > + * @bank: pin bank location > + * @pin: pin register offset > + */ > +struct rz_pin_desc { > + unsigned int bank, pin; > + unsigned int number; > + const char *name; > +}; > + > +/** > + * rz_pinctrl_info - SoC info data > + * > + * @ops: SoC specific operations > + * @npins: number of pins > + * @pins: pin array > + */ > +struct rz_pinctrl_ops; > +struct rz_pinctrl_info { > + struct rz_pinctrl_ops *ops; > + > + unsigned int npins; > + struct rz_pin_desc *pins; > +}; > + > +/** > + * rz_pinctrl_res - Memory resource for RZ SoC > + * > + * @start: physical address base > + * @size: memory region size > + * @base: logical address base > + */ > +struct rz_pinctrl_res { > + resource_size_t start; > + resource_size_t size; > + void __iomem *base; > +}; > + > +/** > + * rz_pinctrl_dev - RZ pincontroller device > + * > + * @dev: device > + * @info: SoC data > + * @nres: number of memory regions > + * @res: memory regions > + * @mutex: protect pin*_generic functions > + * @pins: pin array for pinctrl core > + * @desc: pincontroller desc for pinctrl core > + * @pctl: pinctrl device > + */ > +struct rz_pinctrl_dev { > + struct device *dev; > + > + struct rz_pinctrl_info *info; > + > + unsigned int nres; > + struct rz_pinctrl_res *res; > + > + struct mutex mutex; > + > + struct pinctrl_pin_desc *pins; > + struct pinctrl_desc desc; > + struct pinctrl_dev *pctl; > +}; > + > +/** > + * rz_pinctrl_ops - SoC operations > + * > + * @set_mux: perform pin muxing on SoC registers > + */ > +struct rz_pinctrl_ops { > + int (*set_mux)(struct rz_pinctrl_dev *, struct rz_pin_desc *, > + unsigned int mux_mode); > +}; > + > +/* ------------------------------------------------------------------------ > + * pinctrl-rz prototypes > + */ > +int rz_pinctrl_probe(struct platform_device *pdev, > + struct rz_pinctrl_info *info); > +void rz_pinctrl_remove(struct platform_device *pdev); > + > +#endif -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-02-01 15:21 ` Laurent Pinchart @ 2017-02-06 18:15 ` jacopo mondi 2017-02-06 18:28 ` Tony Lindgren 0 siblings, 1 reply; 54+ messages in thread From: jacopo mondi @ 2017-02-06 18:15 UTC (permalink / raw) To: Laurent Pinchart, linus.walleij, Tony Lindgren Cc: geert+renesas, linux-renesas-soc, linux-gpio Hi Laurent, On 01/02/2017 16:21, Laurent Pinchart wrote: > Hi Jacopo, > [snip] > >> +} >> + >> +/** >> + * rz_dt_node_to_map() - Parse device tree nodes and collect pins, groups >> and >> + * functions > > I don't think we have groups and functions, do we ? > Sort of.. The hardware does not have that, but we create them from pin configuration sub-nodes, as the pinmux core API is sort of group/function centric (see the pinmux_ops.set_mux function prototype) [snip] > + mutex_lock(&rz_pinctrl->mutex); This seems weird. The pinctrl generic functions indeed state that the caller must take care of locking, but there doesn't seem to be any lock protecting races between .dt_node_to_map() calling the generic functions below, and the generic .get_group_*() handlers. > + ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins, NULL); > + if (ret) { > + mutex_unlock(&rz_pinctrl->mutex); > + goto free_map; > + } > + > + ret = pinmux_generic_add_function(pctldev, grpname, fngrps, > + 1, mux_modes); > + if (ret) { > + mutex_unlock(&rz_pinctrl->mutex); > + goto free_map; > + } > + mutex_unlock(&rz_pinctrl->mutex); radix trees are RCU protected structures, so no locking should be required when accessing them from different places, so I guess lock here is just to protect the num_groups and num_functions variables in core pin controller driver... [snip] >> +/** >> + * rz_dt_free_map() >> + */ >> +static void rz_dt_free_map(struct pinctrl_dev *pctldev, >> + struct pinctrl_map *map, unsigned int num_maps) >> +{ >> + struct rz_pinctrl_dev *rz_pinctrl = pinctrl_dev_get_drvdata(pctldev); >> + >> + devm_kfree(rz_pinctrl->dev, map); > > Shouldn't you also free the other allocated pieces of memory (fngrps, > mux_modes and grpins) ? > That's an interesting one. If I have to free all data associated with that map (and I assume so) I shall be able to retrieve groups and functions that map represents and delete them and Currently there is no generic pinctrl and pinmux function to retrieve a group or function by name, but only by their id (selector). It would take 10minutes to add them, but I wonder if that's desirable or there are other ways to do so I haven't found out yet. Linus, Tony and gpio people: do you have opinions on this? I can add those functions to core/pinmux in my next patch series if I get your ack here. Thanks j ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-02-06 18:15 ` jacopo mondi @ 2017-02-06 18:28 ` Tony Lindgren 2017-02-07 10:27 ` jacopo mondi 0 siblings, 1 reply; 54+ messages in thread From: Tony Lindgren @ 2017-02-06 18:28 UTC (permalink / raw) To: jacopo mondi Cc: Laurent Pinchart, linus.walleij, geert+renesas, linux-renesas-soc, linux-gpio * jacopo mondi <jacopo@jmondi.org> [170206 10:16]: > Currently there is no generic pinctrl and pinmux function to retrieve a > group or function by name, but only by their id (selector). > It would take 10minutes to add them, but I wonder if that's desirable or > there are other ways to do so I haven't found out yet. > > Linus, Tony and gpio people: do you have opinions on this? I can add those > functions to core/pinmux in my next patch series if I get your ack here. I would just get rid of the names and allow optionally configuring them from the consumer. Just like we do for interrupts and GPIOs. Regards, Tony ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module 2017-02-06 18:28 ` Tony Lindgren @ 2017-02-07 10:27 ` jacopo mondi 0 siblings, 0 replies; 54+ messages in thread From: jacopo mondi @ 2017-02-07 10:27 UTC (permalink / raw) To: Tony Lindgren Cc: Laurent Pinchart, linus.walleij, geert+renesas, linux-renesas-soc, linux-gpio Hi Tony, On 06/02/2017 19:28, Tony Lindgren wrote: > * jacopo mondi <jacopo@jmondi.org> [170206 10:16]: >> Currently there is no generic pinctrl and pinmux function to retrieve a >> group or function by name, but only by their id (selector). >> It would take 10minutes to add them, but I wonder if that's desirable or >> there are other ways to do so I haven't found out yet. >> >> Linus, Tony and gpio people: do you have opinions on this? I can add those >> functions to core/pinmux in my next patch series if I get your ack here. > > I would just get rid of the names and allow optionally configuring them > from the consumer. Just like we do for interrupts and GPIOs. > Mostly, I wonder why so far nobody had to remove function and groups and release their associated data when deleting a map. It makes me think this is not required or I didn't fully get dt_free_map() purpose :/ Or, put it in another way, I see lot of code releasing memory associated with PIN_MAP_TYPE_CONFIGS_GROUP map types (as map.data.configs.configs it's easy accessible), but none releasing data associated to PIN_MAP_TYPE_MUX_GROUP map types... Thanks j > Regards, > > Tony > ^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi 2017-01-25 18:09 ` [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module Jacopo Mondi @ 2017-01-25 18:09 ` Jacopo Mondi 2017-01-26 19:49 ` Geert Uytterhoeven ` (2 more replies) 2017-01-25 18:09 ` [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header Jacopo Mondi ` (6 subsequent siblings) 8 siblings, 3 replies; 54+ messages in thread From: Jacopo Mondi @ 2017-01-25 18:09 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Add pin controller driver for Renesas RZ/A1 SoC. The SoC driver registers to rz-pfc core module and provides pin description array and SoC specific pin mux operation. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/pinctrl/rz-pfc/Kconfig | 7 + drivers/pinctrl/rz-pfc/Makefile | 1 + drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 347 ++++++++++++++++++++++++++++++++++ 3 files changed, 355 insertions(+) create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rza1.c diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig index 3714c10..2b9c111 100644 --- a/drivers/pinctrl/rz-pfc/Kconfig +++ b/drivers/pinctrl/rz-pfc/Kconfig @@ -15,4 +15,11 @@ config PINCTRL_RZ_PINCTRL help This enables pin control drivers for Renesas RZ platforms +config PINCTRL_RZA1_PINCTRL + depends on ARCH_R7S72100 + select PINCTRL_RZ_PINCTRL + def_bool y + help + This enables pin control drivers for Renesas RZ/A1 SoC + endif diff --git a/drivers/pinctrl/rz-pfc/Makefile b/drivers/pinctrl/rz-pfc/Makefile index cba8283..e3befa5 100644 --- a/drivers/pinctrl/rz-pfc/Makefile +++ b/drivers/pinctrl/rz-pfc/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_PINCTRL_RZ_PINCTRL) += pinctrl-rz.o +obj-$(CONFIG_PINCTRL_RZA1_PINCTRL) += pinctrl-rza1.o diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c new file mode 100644 index 0000000..221f048 --- /dev/null +++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c @@ -0,0 +1,347 @@ +/* + * Pinctrl support for Renesas RZ/A1 SoC + * + * Copyright (C) 2017 Jacopo Mondi + * Copyright (C) 2017 Renesas Electronics Corporation + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ +#include <linux/err.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/types.h> + +#include <linux/of.h> +#include <linux/of_device.h> + +#include "pinctrl-rz.h" + +#define RZA1_REG_DBG + +/* memory resources indexes */ +#define MEM_RES_LOW 0 /* PORTn_base */ +#define MEM_RES_HIGH 1 /* PORTn_base + 0x4000 */ + +/* displacements from PORTn_base */ +#define PMC_REG 0x400 +#define PFC_REG 0x500 +#define PFCE_REG 0x600 +#define PFCEA_REG 0xA00 + +/* displacements from PORTn_base + 0x4000 */ +#define PIBC_REG 0x000 +#define PBDC_REG 0x100 +#define PIPC_REG 0x200 + +/* build register address using memory base, offset and bank number */ +#define RZA1_ADDR(mem_base, reg, bank) \ + ((mem_base) + (reg) + (bank * 4)) + +/* ---------------------------------------------------------------------------- + * pin enumeration + */ +enum rza1_pins { + /* Port 0 */ + RZ_PIN_NAME(0, 0) = 0, RZ_PIN_NAME(0, 1), RZ_PIN_NAME(0, 2), + RZ_PIN_NAME(0, 3), RZ_PIN_NAME(0, 4), RZ_PIN_NAME(0, 5), + + /* Port 1 */ + RZ_PIN_NAME(1, 0), RZ_PIN_NAME(1, 1), RZ_PIN_NAME(1, 2), + RZ_PIN_NAME(1, 3), RZ_PIN_NAME(1, 4), RZ_PIN_NAME(1, 5), + RZ_PIN_NAME(1, 6), RZ_PIN_NAME(1, 7), RZ_PIN_NAME(1, 8), + RZ_PIN_NAME(1, 9), RZ_PIN_NAME(1, 10), RZ_PIN_NAME(1, 11), + RZ_PIN_NAME(1, 12), RZ_PIN_NAME(1, 13), RZ_PIN_NAME(1, 14), + RZ_PIN_NAME(1, 15), + + /* Port 2 */ + RZ_PIN_NAME(2, 0), RZ_PIN_NAME(2, 1), RZ_PIN_NAME(2, 2), + RZ_PIN_NAME(2, 3), RZ_PIN_NAME(2, 4), RZ_PIN_NAME(2, 5), + RZ_PIN_NAME(2, 6), RZ_PIN_NAME(2, 7), RZ_PIN_NAME(2, 8), + RZ_PIN_NAME(2, 9), RZ_PIN_NAME(2, 10), RZ_PIN_NAME(2, 11), + RZ_PIN_NAME(2, 12), RZ_PIN_NAME(2, 13), RZ_PIN_NAME(2, 14), + RZ_PIN_NAME(2, 15), + + /* Port 3 */ + RZ_PIN_NAME(3, 0), RZ_PIN_NAME(3, 1), RZ_PIN_NAME(3, 2), + RZ_PIN_NAME(3, 3), RZ_PIN_NAME(3, 4), RZ_PIN_NAME(3, 5), + RZ_PIN_NAME(3, 6), RZ_PIN_NAME(3, 7), RZ_PIN_NAME(3, 8), + RZ_PIN_NAME(3, 9), RZ_PIN_NAME(3, 10), RZ_PIN_NAME(3, 11), + RZ_PIN_NAME(3, 12), RZ_PIN_NAME(3, 13), RZ_PIN_NAME(3, 14), + RZ_PIN_NAME(3, 15), + + /* Port 4 */ + RZ_PIN_NAME(4, 0), RZ_PIN_NAME(4, 1), RZ_PIN_NAME(4, 2), + RZ_PIN_NAME(4, 3), RZ_PIN_NAME(4, 4), RZ_PIN_NAME(4, 5), + RZ_PIN_NAME(4, 6), RZ_PIN_NAME(4, 7), RZ_PIN_NAME(4, 8), + RZ_PIN_NAME(4, 9), RZ_PIN_NAME(4, 10), RZ_PIN_NAME(4, 11), + RZ_PIN_NAME(4, 12), RZ_PIN_NAME(4, 13), RZ_PIN_NAME(4, 14), + RZ_PIN_NAME(4, 15), + + /* Port 5 */ + RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2), + RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5), + RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8), + RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10), + + /* Port 6 */ + RZ_PIN_NAME(6, 0), RZ_PIN_NAME(6, 1), RZ_PIN_NAME(6, 2), + RZ_PIN_NAME(6, 3), RZ_PIN_NAME(6, 4), RZ_PIN_NAME(6, 5), + RZ_PIN_NAME(6, 6), RZ_PIN_NAME(6, 7), RZ_PIN_NAME(6, 8), + RZ_PIN_NAME(6, 9), RZ_PIN_NAME(6, 10), RZ_PIN_NAME(6, 11), + RZ_PIN_NAME(6, 12), RZ_PIN_NAME(6, 13), RZ_PIN_NAME(6, 14), + RZ_PIN_NAME(6, 15), + + /* Port 7 */ + RZ_PIN_NAME(7, 0), RZ_PIN_NAME(7, 1), RZ_PIN_NAME(7, 2), + RZ_PIN_NAME(7, 3), RZ_PIN_NAME(7, 4), RZ_PIN_NAME(7, 5), + RZ_PIN_NAME(7, 6), RZ_PIN_NAME(7, 7), RZ_PIN_NAME(7, 8), + RZ_PIN_NAME(7, 9), RZ_PIN_NAME(7, 10), RZ_PIN_NAME(7, 11), + RZ_PIN_NAME(7, 12), RZ_PIN_NAME(7, 13), RZ_PIN_NAME(7, 14), + RZ_PIN_NAME(7, 15), + + /* Port 8 */ + RZ_PIN_NAME(8, 0), RZ_PIN_NAME(8, 1), RZ_PIN_NAME(8, 2), + RZ_PIN_NAME(8, 3), RZ_PIN_NAME(8, 4), RZ_PIN_NAME(8, 5), + RZ_PIN_NAME(8, 6), RZ_PIN_NAME(8, 7), RZ_PIN_NAME(8, 8), + RZ_PIN_NAME(8, 9), RZ_PIN_NAME(8, 10), RZ_PIN_NAME(8, 11), + RZ_PIN_NAME(8, 12), RZ_PIN_NAME(8, 13), RZ_PIN_NAME(8, 14), + RZ_PIN_NAME(8, 15), + + /* Port 9 */ + RZ_PIN_NAME(9, 0), RZ_PIN_NAME(9, 1), RZ_PIN_NAME(9, 2), + RZ_PIN_NAME(9, 3), RZ_PIN_NAME(9, 4), RZ_PIN_NAME(9, 5), + RZ_PIN_NAME(9, 6), RZ_PIN_NAME(9, 7), + + /* Port 10 */ + RZ_PIN_NAME(10, 0), RZ_PIN_NAME(10, 1), RZ_PIN_NAME(10, 2), + RZ_PIN_NAME(10, 3), RZ_PIN_NAME(10, 4), RZ_PIN_NAME(10, 5), + RZ_PIN_NAME(10, 6), RZ_PIN_NAME(10, 7), RZ_PIN_NAME(10, 8), + RZ_PIN_NAME(10, 9), RZ_PIN_NAME(10, 10), RZ_PIN_NAME(10, 11), + RZ_PIN_NAME(10, 12), RZ_PIN_NAME(10, 13), RZ_PIN_NAME(10, 14), + RZ_PIN_NAME(10, 15), + + /* Port 10 */ + RZ_PIN_NAME(11, 0), RZ_PIN_NAME(11, 1), RZ_PIN_NAME(11, 2), + RZ_PIN_NAME(11, 3), RZ_PIN_NAME(11, 4), RZ_PIN_NAME(11, 5), + RZ_PIN_NAME(11, 6), RZ_PIN_NAME(11, 7), RZ_PIN_NAME(11, 8), + RZ_PIN_NAME(11, 9), RZ_PIN_NAME(11, 10), RZ_PIN_NAME(11, 11), + RZ_PIN_NAME(11, 12), RZ_PIN_NAME(11, 13), RZ_PIN_NAME(11, 14), + RZ_PIN_NAME(11, 15), +}; + +struct rz_pin_desc pins[] = { + /* Port 0 */ + RZ_PIN_DESC(0, 0), RZ_PIN_DESC(0, 1), RZ_PIN_DESC(0, 2), + RZ_PIN_DESC(0, 3), RZ_PIN_DESC(0, 4), RZ_PIN_DESC(0, 5), + + /* Port 1 */ + RZ_PIN_DESC(1, 0), RZ_PIN_DESC(1, 1), RZ_PIN_DESC(1, 2), + RZ_PIN_DESC(1, 3), RZ_PIN_DESC(1, 4), RZ_PIN_DESC(1, 5), + RZ_PIN_DESC(1, 6), RZ_PIN_DESC(1, 7), RZ_PIN_DESC(1, 8), + RZ_PIN_DESC(1, 9), RZ_PIN_DESC(1, 10), RZ_PIN_DESC(1, 11), + RZ_PIN_DESC(1, 12), RZ_PIN_DESC(1, 13), RZ_PIN_DESC(1, 14), + RZ_PIN_DESC(1, 15), + + /* Port 2 */ + RZ_PIN_DESC(2, 0), RZ_PIN_DESC(2, 1), RZ_PIN_DESC(2, 2), + RZ_PIN_DESC(2, 3), RZ_PIN_DESC(2, 4), RZ_PIN_DESC(2, 5), + RZ_PIN_DESC(2, 6), RZ_PIN_DESC(2, 7), RZ_PIN_DESC(2, 8), + RZ_PIN_DESC(2, 9), RZ_PIN_DESC(2, 10), RZ_PIN_DESC(2, 11), + RZ_PIN_DESC(2, 12), RZ_PIN_DESC(2, 13), RZ_PIN_DESC(2, 14), + RZ_PIN_DESC(2, 15), + + /* Port 3 */ + RZ_PIN_DESC(3, 0), RZ_PIN_DESC(3, 1), RZ_PIN_DESC(3, 2), + RZ_PIN_DESC(3, 3), RZ_PIN_DESC(3, 4), RZ_PIN_DESC(3, 5), + RZ_PIN_DESC(3, 6), RZ_PIN_DESC(3, 7), RZ_PIN_DESC(3, 8), + RZ_PIN_DESC(3, 9), RZ_PIN_DESC(3, 10), RZ_PIN_DESC(3, 11), + RZ_PIN_DESC(3, 12), RZ_PIN_DESC(3, 13), RZ_PIN_DESC(3, 14), + RZ_PIN_DESC(3, 15), + + /* Port 4 */ + RZ_PIN_DESC(4, 0), RZ_PIN_DESC(4, 1), RZ_PIN_DESC(4, 2), + RZ_PIN_DESC(4, 3), RZ_PIN_DESC(4, 4), RZ_PIN_DESC(4, 5), + RZ_PIN_DESC(4, 6), RZ_PIN_DESC(4, 7), RZ_PIN_DESC(4, 8), + RZ_PIN_DESC(4, 9), RZ_PIN_DESC(4, 10), RZ_PIN_DESC(4, 11), + RZ_PIN_DESC(4, 12), RZ_PIN_DESC(4, 13), RZ_PIN_DESC(4, 14), + RZ_PIN_DESC(4, 15), + + /* Port 5 */ + RZ_PIN_DESC(5, 0), RZ_PIN_DESC(5, 1), RZ_PIN_DESC(5, 2), + RZ_PIN_DESC(5, 3), RZ_PIN_DESC(5, 4), RZ_PIN_DESC(5, 5), + RZ_PIN_DESC(5, 6), RZ_PIN_DESC(5, 7), RZ_PIN_DESC(5, 8), + RZ_PIN_DESC(5, 9), RZ_PIN_DESC(5, 10), + + /* Port 6 */ + RZ_PIN_DESC(6, 0), RZ_PIN_DESC(6, 1), RZ_PIN_DESC(6, 2), + RZ_PIN_DESC(6, 3), RZ_PIN_DESC(6, 4), RZ_PIN_DESC(6, 5), + RZ_PIN_DESC(6, 6), RZ_PIN_DESC(6, 7), RZ_PIN_DESC(6, 8), + RZ_PIN_DESC(6, 9), RZ_PIN_DESC(6, 10), RZ_PIN_DESC(6, 11), + RZ_PIN_DESC(6, 12), RZ_PIN_DESC(6, 13), RZ_PIN_DESC(6, 14), + RZ_PIN_DESC(6, 15), + + /* Port 7 */ + RZ_PIN_DESC(7, 0), RZ_PIN_DESC(7, 1), RZ_PIN_DESC(7, 2), + RZ_PIN_DESC(7, 3), RZ_PIN_DESC(7, 4), RZ_PIN_DESC(7, 5), + RZ_PIN_DESC(7, 6), RZ_PIN_DESC(7, 7), RZ_PIN_DESC(7, 8), + RZ_PIN_DESC(7, 9), RZ_PIN_DESC(7, 10), RZ_PIN_DESC(7, 11), + RZ_PIN_DESC(7, 12), RZ_PIN_DESC(7, 13), RZ_PIN_DESC(7, 14), + RZ_PIN_DESC(7, 15), + + /* Port 8 */ + RZ_PIN_DESC(8, 0), RZ_PIN_DESC(8, 1), RZ_PIN_DESC(8, 2), + RZ_PIN_DESC(8, 3), RZ_PIN_DESC(8, 4), RZ_PIN_DESC(8, 5), + RZ_PIN_DESC(8, 6), RZ_PIN_DESC(8, 7), RZ_PIN_DESC(8, 8), + RZ_PIN_DESC(8, 9), RZ_PIN_DESC(8, 10), RZ_PIN_DESC(8, 11), + RZ_PIN_DESC(8, 12), RZ_PIN_DESC(8, 13), RZ_PIN_DESC(8, 14), + RZ_PIN_DESC(8, 15), + + /* Port 9 */ + RZ_PIN_DESC(9, 0), RZ_PIN_DESC(9, 1), RZ_PIN_DESC(9, 2), + RZ_PIN_DESC(9, 3), RZ_PIN_DESC(9, 4), RZ_PIN_DESC(9, 5), + RZ_PIN_DESC(9, 6), RZ_PIN_DESC(9, 7), + + /* Port 10 */ + RZ_PIN_DESC(10, 0), RZ_PIN_DESC(10, 1), RZ_PIN_DESC(10, 2), + RZ_PIN_DESC(10, 3), RZ_PIN_DESC(10, 4), RZ_PIN_DESC(10, 5), + RZ_PIN_DESC(10, 6), RZ_PIN_DESC(10, 7), RZ_PIN_DESC(10, 8), + RZ_PIN_DESC(10, 9), RZ_PIN_DESC(10, 10), RZ_PIN_DESC(10, 11), + RZ_PIN_DESC(10, 12), RZ_PIN_DESC(10, 13), RZ_PIN_DESC(10, 14), + RZ_PIN_DESC(10, 15), + + /* Port 11 */ + RZ_PIN_DESC(11, 0), RZ_PIN_DESC(11, 1), RZ_PIN_DESC(11, 2), + RZ_PIN_DESC(11, 3), RZ_PIN_DESC(11, 4), RZ_PIN_DESC(11, 5), + RZ_PIN_DESC(11, 6), RZ_PIN_DESC(11, 7), RZ_PIN_DESC(11, 8), + RZ_PIN_DESC(11, 9), RZ_PIN_DESC(11, 10), RZ_PIN_DESC(11, 11), + RZ_PIN_DESC(11, 12), RZ_PIN_DESC(11, 13), RZ_PIN_DESC(11, 14), + RZ_PIN_DESC(11, 15), +}; + +/* ---------------------------------------------------------------------------- + * SoC operations + */ + +static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg, + int bank, int pin, int set) +{ + void __iomem *mem = RZA1_ADDR(res->base, reg, bank); + u16 val = set ? ioread16(mem) | 1 << pin : + ioread16(mem) & ~(1 << pin); +#ifdef RZA1_REG_DBG + u16 temp = ioread16(mem); + + pr_err("%p %p %p - %4x %4x\n", + (void *)res->start, res->base, mem, temp, val); +#endif + iowrite16(val, mem); +} + +/** + * rza1_set_mux() - Configure alternate function settings for the selected pin + * + * @pinctrl: RZ pincontroller + * @pin: Pin to configure + * @mux_mode: Alternate function number + * + * @return: 0 for success; != 0 otherwise + */ +static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl, + struct rz_pin_desc *pin_desc, unsigned int mux_mode) +{ + struct rz_pinctrl_res *res; + unsigned int bank = pin_desc->bank, + pin = pin_desc->pin; + + /* + * disable input buffer and bi-control direction before entering + * alternate mode and let alternate function drive the IO mode by + * setting PIPCn to 1 + */ + res = &pinctrl->res[MEM_RES_HIGH]; + rza1_set_bit(res, PIBC_REG, bank, pin, 0); + rza1_set_bit(res, PBDC_REG, bank, pin, 0); + rza1_set_bit(res, PIPC_REG, bank, pin, 1); + + /* TODO: + * all alternate functions except a few (3) need PIPCn = 1; + * find a way to identify those 3 functions, do not set PIPCn to 1 + * and set PMn according to some flag passed as parameter from DTS + */ + + /* + * enable alternate function mode and select it. + * + * ---------------------------------------------------- + * Alternate mode selection table: + * + * PMC PFC PFCE PFCAE AlternateMode mux_mode + * 1 0 0 0 1 0 + * 1 1 0 0 2 1 + * 1 0 1 0 3 2 + * 1 1 1 0 4 3 + * 1 0 0 1 5 4 + * 1 1 0 1 6 5 + * 1 0 1 1 7 6 + * 1 1 1 1 8 7 + * ---------------------------------------------------- + */ + res = &pinctrl->res[MEM_RES_LOW]; + rza1_set_bit(res, PMC_REG, bank, pin, 1); + rza1_set_bit(res, PFC_REG, bank, pin, mux_mode & 0x1); + rza1_set_bit(res, PFCE_REG, bank, pin, mux_mode & 0x2); + rza1_set_bit(res, PFCEA_REG, bank, pin, mux_mode & 0x4); + + return 0; +} + +static struct rz_pinctrl_ops rza1_pinctrl_ops = { + .set_mux = rza1_set_mux, +}; + +static struct rz_pinctrl_info rza1_info = { + .ops = &rza1_pinctrl_ops, + + .npins = ARRAY_SIZE(pins), + .pins = pins, +}; + +static const struct of_device_id rza1_pinctrl_of_match[] = { + { .compatible = "renesas,rza1-pinctrl", }, + { } +}; + +static int rza1_pinctrl_probe(struct platform_device *pdev) +{ + return rz_pinctrl_probe(pdev, &rza1_info); +} + +static int rza1_pinctrl_remove(struct platform_device *pdev) +{ + rz_pinctrl_remove(pdev); + + return 0; +} + +static struct platform_driver rza1_pinctrl_driver = { + .driver = { + .name = "rza1_pinctrl_driver", + .of_match_table = rza1_pinctrl_of_match, + }, + .probe = rza1_pinctrl_probe, + .remove = rza1_pinctrl_remove, +}; + +static int __init rza1_pinctrl_init(void) +{ + return platform_driver_register(&rza1_pinctrl_driver); +} +core_initcall(rza1_pinctrl_init); + +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org"); +MODULE_DESCRIPTION("Pinctl driver for Reneas RZ/A1 SoC"); +MODULE_LICENSE("GPL v2"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver 2017-01-25 18:09 ` [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver Jacopo Mondi @ 2017-01-26 19:49 ` Geert Uytterhoeven 2017-01-30 19:19 ` Chris Brandt 2017-02-01 15:25 ` Laurent Pinchart 2 siblings, 0 replies; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-26 19:49 UTC (permalink / raw) To: Jacopo Mondi Cc: Laurent Pinchart, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Jacopo, On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > Add pin controller driver for Renesas RZ/A1 SoC. > The SoC driver registers to rz-pfc core module and provides pin > description array and SoC specific pin mux operation. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- a/drivers/pinctrl/rz-pfc/Makefile > +++ b/drivers/pinctrl/rz-pfc/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_PINCTRL_RZ_PINCTRL) += pinctrl-rz.o > +obj-$(CONFIG_PINCTRL_RZA1_PINCTRL) += pinctrl-rza1.o > diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c > new file mode 100644 > index 0000000..221f048 > --- /dev/null > +++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c > + > +/* ---------------------------------------------------------------------------- > + * SoC operations > + */ > + > +static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg, > + int bank, int pin, int set) > +{ > + void __iomem *mem = RZA1_ADDR(res->base, reg, bank); > + u16 val = set ? ioread16(mem) | 1 << pin : > + ioread16(mem) & ~(1 << pin); > +#ifdef RZA1_REG_DBG > + u16 temp = ioread16(mem); > + > + pr_err("%p %p %p - %4x %4x\n", > + (void *)res->start, res->base, mem, temp, val); Please drop the cast, take the address, and use %pa to format a resource_size_t, cfr. Documentation/printk-formats.txt. > + static struct rz_pinctrl_ops rza1_pinctrl_ops = { const > + .set_mux = rza1_set_mux, > +}; > + > +static struct rz_pinctrl_info rza1_info = { const Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver 2017-01-25 18:09 ` [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver Jacopo Mondi 2017-01-26 19:49 ` Geert Uytterhoeven @ 2017-01-30 19:19 ` Chris Brandt 2017-01-31 10:39 ` Laurent Pinchart 2017-02-01 15:25 ` Laurent Pinchart 2 siblings, 1 reply; 54+ messages in thread From: Chris Brandt @ 2017-01-30 19:19 UTC (permalink / raw) To: Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hi Jacopo, On Wednesday, January 25, 2017, Jacopo Mondi wrote: > + /* Port 5 */ > + RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2), > + RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5), > + RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8), > + RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10), The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses all 16 port pins on "port 5" so I'd like to include them as well. > +static const struct of_device_id rza1_pinctrl_of_match[] = { > + { .compatible = "renesas,rza1-pinctrl", }, > + { } > +}; Since this PFC driver file is specifically for RZ/A1, I think a better compatible string would be: .compatible = "renesas,r7s72100-renesas-pinctrl", Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver 2017-01-30 19:19 ` Chris Brandt @ 2017-01-31 10:39 ` Laurent Pinchart 2017-01-31 14:11 ` Chris Brandt 0 siblings, 1 reply; 54+ messages in thread From: Laurent Pinchart @ 2017-01-31 10:39 UTC (permalink / raw) To: Chris Brandt Cc: Jacopo Mondi, geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Chris, On Monday 30 Jan 2017 19:19:18 Chris Brandt wrote: > On Wednesday, January 25, 2017, Jacopo Mondi wrote: > > + /* Port 5 */ > > + RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2), > > + RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5), > > + RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8), > > + RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10), > > The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses > all 16 port pins on "port 5" so I'd like to include them as well. > > > +static const struct of_device_id rza1_pinctrl_of_match[] = { > > + { .compatible = "renesas,rza1-pinctrl", }, > > + { } > > +}; > > Since this PFC driver file is specifically for RZ/A1, I think a > better compatible string would be: > > .compatible = "renesas,r7s72100-renesas-pinctrl", Do we need to repeat "renesas" in the name ? And given that the datasheet names the hardware "ports", how about "renesas,r7s72100-ports" ? The IP core handles both pinctrl and GPIO, so "pinctrl" is a bit restrictive. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver 2017-01-31 10:39 ` Laurent Pinchart @ 2017-01-31 14:11 ` Chris Brandt 0 siblings, 0 replies; 54+ messages in thread From: Chris Brandt @ 2017-01-31 14:11 UTC (permalink / raw) To: Laurent Pinchart Cc: Jacopo Mondi, geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Laurent, On Tuesday, January 31, 2017, Laurent Pinchart wrote: > On Monday 30 Jan 2017 19:19:18 Chris Brandt wrote: > > On Wednesday, January 25, 2017, Jacopo Mondi wrote: > > > + /* Port 5 */ > > > + RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2), > > > + RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5), > > > + RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8), > > > + RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10), > > > > The RZ/A1L (basically a subset of RZ/A1H to reduce cost) uses all 16 > > port pins on "port 5" so I'd like to include them as well. > > > > > +static const struct of_device_id rza1_pinctrl_of_match[] = { > > > + { .compatible = "renesas,rza1-pinctrl", }, > > > + { } > > > +}; > > > > Since this PFC driver file is specifically for RZ/A1, I think a better > > compatible string would be: > > > > .compatible = "renesas,r7s72100-renesas-pinctrl", > > Do we need to repeat "renesas" in the name ? And given that the datasheet > names the hardware "ports", how about "renesas,r7s72100-ports" ? The IP > core handles both pinctrl and GPIO, so "pinctrl" is a bit restrictive. Personally, I do not like "renesas,r7s72100-renesas-pinctrl". I was simply trying to follow the naming guidelines for DT. I like your suggestion of "renesas,r7s72100-ports" better. Cheers Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver 2017-01-25 18:09 ` [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver Jacopo Mondi 2017-01-26 19:49 ` Geert Uytterhoeven 2017-01-30 19:19 ` Chris Brandt @ 2017-02-01 15:25 ` Laurent Pinchart 2 siblings, 0 replies; 54+ messages in thread From: Laurent Pinchart @ 2017-02-01 15:25 UTC (permalink / raw) To: Jacopo Mondi; +Cc: geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Jacopo, Thank you for the patch. On Wednesday 25 Jan 2017 19:09:44 Jacopo Mondi wrote: > Add pin controller driver for Renesas RZ/A1 SoC. > The SoC driver registers to rz-pfc core module and provides pin > description array and SoC specific pin mux operation. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/pinctrl/rz-pfc/Kconfig | 7 + > drivers/pinctrl/rz-pfc/Makefile | 1 + > drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 347 +++++++++++++++++++++++++++++++ > 3 files changed, 355 insertions(+) > create mode 100644 drivers/pinctrl/rz-pfc/pinctrl-rza1.c > > diff --git a/drivers/pinctrl/rz-pfc/Kconfig b/drivers/pinctrl/rz-pfc/Kconfig > index 3714c10..2b9c111 100644 > --- a/drivers/pinctrl/rz-pfc/Kconfig > +++ b/drivers/pinctrl/rz-pfc/Kconfig > @@ -15,4 +15,11 @@ config PINCTRL_RZ_PINCTRL > help > This enables pin control drivers for Renesas RZ platforms > > +config PINCTRL_RZA1_PINCTRL > + depends on ARCH_R7S72100 > + select PINCTRL_RZ_PINCTRL > + def_bool y > + help > + This enables pin control drivers for Renesas RZ/A1 SoC > + > endif > diff --git a/drivers/pinctrl/rz-pfc/Makefile > b/drivers/pinctrl/rz-pfc/Makefile index cba8283..e3befa5 100644 > --- a/drivers/pinctrl/rz-pfc/Makefile > +++ b/drivers/pinctrl/rz-pfc/Makefile > @@ -1 +1,2 @@ > obj-$(CONFIG_PINCTRL_RZ_PINCTRL) += pinctrl-rz.o > +obj-$(CONFIG_PINCTRL_RZA1_PINCTRL) += pinctrl-rza1.o > diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c > b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c new file mode 100644 > index 0000000..221f048 > --- /dev/null > +++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c > @@ -0,0 +1,347 @@ > +/* > + * Pinctrl support for Renesas RZ/A1 SoC > + * > + * Copyright (C) 2017 Jacopo Mondi > + * Copyright (C) 2017 Renesas Electronics Corporation > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > + > +#include <linux/of.h> > +#include <linux/of_device.h> You can move those two lines between module.h and platform_device.h, there's no need to keep them separate. > + > +#include "pinctrl-rz.h" > + > +#define RZA1_REG_DBG > + > +/* memory resources indexes */ > +#define MEM_RES_LOW 0 /* PORTn_base */ > +#define MEM_RES_HIGH 1 /* PORTn_base + 0x4000 */ > + > +/* displacements from PORTn_base */ > +#define PMC_REG 0x400 > +#define PFC_REG 0x500 > +#define PFCE_REG 0x600 > +#define PFCEA_REG 0xA00 The kernel mostly uses lowercase for hex values. > + > +/* displacements from PORTn_base + 0x4000 */ > +#define PIBC_REG 0x000 > +#define PBDC_REG 0x100 > +#define PIPC_REG 0x200 > + > +/* build register address using memory base, offset and bank number */ > +#define RZA1_ADDR(mem_base, reg, bank) \ > + ((mem_base) + (reg) + (bank * 4)) You should put parentheses around bank: ((mem_base) + (reg) + (bank) * 4) to avoid side effects if the bank parameter isn't a constant (that's not the case here, but it's a good practice anyway). > + > +/* ------------------------------------------------------------------------ > + * pin enumeration > + */ > +enum rza1_pins { > + /* Port 0 */ > + RZ_PIN_NAME(0, 0) = 0, RZ_PIN_NAME(0, 1), RZ_PIN_NAME(0, 2), Is there a need for = 0 ? > + RZ_PIN_NAME(0, 3), RZ_PIN_NAME(0, 4), RZ_PIN_NAME(0, 5), > + > + /* Port 1 */ > + RZ_PIN_NAME(1, 0), RZ_PIN_NAME(1, 1), RZ_PIN_NAME(1, 2), > + RZ_PIN_NAME(1, 3), RZ_PIN_NAME(1, 4), RZ_PIN_NAME(1, 5), > + RZ_PIN_NAME(1, 6), RZ_PIN_NAME(1, 7), RZ_PIN_NAME(1, 8), > + RZ_PIN_NAME(1, 9), RZ_PIN_NAME(1, 10), RZ_PIN_NAME(1, 11), > + RZ_PIN_NAME(1, 12), RZ_PIN_NAME(1, 13), RZ_PIN_NAME(1, 14), > + RZ_PIN_NAME(1, 15), > + > + /* Port 2 */ > + RZ_PIN_NAME(2, 0), RZ_PIN_NAME(2, 1), RZ_PIN_NAME(2, 2), > + RZ_PIN_NAME(2, 3), RZ_PIN_NAME(2, 4), RZ_PIN_NAME(2, 5), > + RZ_PIN_NAME(2, 6), RZ_PIN_NAME(2, 7), RZ_PIN_NAME(2, 8), > + RZ_PIN_NAME(2, 9), RZ_PIN_NAME(2, 10), RZ_PIN_NAME(2, 11), > + RZ_PIN_NAME(2, 12), RZ_PIN_NAME(2, 13), RZ_PIN_NAME(2, 14), > + RZ_PIN_NAME(2, 15), > + > + /* Port 3 */ > + RZ_PIN_NAME(3, 0), RZ_PIN_NAME(3, 1), RZ_PIN_NAME(3, 2), > + RZ_PIN_NAME(3, 3), RZ_PIN_NAME(3, 4), RZ_PIN_NAME(3, 5), > + RZ_PIN_NAME(3, 6), RZ_PIN_NAME(3, 7), RZ_PIN_NAME(3, 8), > + RZ_PIN_NAME(3, 9), RZ_PIN_NAME(3, 10), RZ_PIN_NAME(3, 11), > + RZ_PIN_NAME(3, 12), RZ_PIN_NAME(3, 13), RZ_PIN_NAME(3, 14), > + RZ_PIN_NAME(3, 15), > + > + /* Port 4 */ > + RZ_PIN_NAME(4, 0), RZ_PIN_NAME(4, 1), RZ_PIN_NAME(4, 2), > + RZ_PIN_NAME(4, 3), RZ_PIN_NAME(4, 4), RZ_PIN_NAME(4, 5), > + RZ_PIN_NAME(4, 6), RZ_PIN_NAME(4, 7), RZ_PIN_NAME(4, 8), > + RZ_PIN_NAME(4, 9), RZ_PIN_NAME(4, 10), RZ_PIN_NAME(4, 11), > + RZ_PIN_NAME(4, 12), RZ_PIN_NAME(4, 13), RZ_PIN_NAME(4, 14), > + RZ_PIN_NAME(4, 15), > + > + /* Port 5 */ > + RZ_PIN_NAME(5, 0), RZ_PIN_NAME(5, 1), RZ_PIN_NAME(5, 2), > + RZ_PIN_NAME(5, 3), RZ_PIN_NAME(5, 4), RZ_PIN_NAME(5, 5), > + RZ_PIN_NAME(5, 6), RZ_PIN_NAME(5, 7), RZ_PIN_NAME(5, 8), > + RZ_PIN_NAME(5, 9), RZ_PIN_NAME(5, 10), > + > + /* Port 6 */ > + RZ_PIN_NAME(6, 0), RZ_PIN_NAME(6, 1), RZ_PIN_NAME(6, 2), > + RZ_PIN_NAME(6, 3), RZ_PIN_NAME(6, 4), RZ_PIN_NAME(6, 5), > + RZ_PIN_NAME(6, 6), RZ_PIN_NAME(6, 7), RZ_PIN_NAME(6, 8), > + RZ_PIN_NAME(6, 9), RZ_PIN_NAME(6, 10), RZ_PIN_NAME(6, 11), > + RZ_PIN_NAME(6, 12), RZ_PIN_NAME(6, 13), RZ_PIN_NAME(6, 14), > + RZ_PIN_NAME(6, 15), > + > + /* Port 7 */ > + RZ_PIN_NAME(7, 0), RZ_PIN_NAME(7, 1), RZ_PIN_NAME(7, 2), > + RZ_PIN_NAME(7, 3), RZ_PIN_NAME(7, 4), RZ_PIN_NAME(7, 5), > + RZ_PIN_NAME(7, 6), RZ_PIN_NAME(7, 7), RZ_PIN_NAME(7, 8), > + RZ_PIN_NAME(7, 9), RZ_PIN_NAME(7, 10), RZ_PIN_NAME(7, 11), > + RZ_PIN_NAME(7, 12), RZ_PIN_NAME(7, 13), RZ_PIN_NAME(7, 14), > + RZ_PIN_NAME(7, 15), > + > + /* Port 8 */ > + RZ_PIN_NAME(8, 0), RZ_PIN_NAME(8, 1), RZ_PIN_NAME(8, 2), > + RZ_PIN_NAME(8, 3), RZ_PIN_NAME(8, 4), RZ_PIN_NAME(8, 5), > + RZ_PIN_NAME(8, 6), RZ_PIN_NAME(8, 7), RZ_PIN_NAME(8, 8), > + RZ_PIN_NAME(8, 9), RZ_PIN_NAME(8, 10), RZ_PIN_NAME(8, 11), > + RZ_PIN_NAME(8, 12), RZ_PIN_NAME(8, 13), RZ_PIN_NAME(8, 14), > + RZ_PIN_NAME(8, 15), > + > + /* Port 9 */ > + RZ_PIN_NAME(9, 0), RZ_PIN_NAME(9, 1), RZ_PIN_NAME(9, 2), > + RZ_PIN_NAME(9, 3), RZ_PIN_NAME(9, 4), RZ_PIN_NAME(9, 5), > + RZ_PIN_NAME(9, 6), RZ_PIN_NAME(9, 7), > + > + /* Port 10 */ > + RZ_PIN_NAME(10, 0), RZ_PIN_NAME(10, 1), RZ_PIN_NAME(10, 2), > + RZ_PIN_NAME(10, 3), RZ_PIN_NAME(10, 4), RZ_PIN_NAME(10, 5), > + RZ_PIN_NAME(10, 6), RZ_PIN_NAME(10, 7), RZ_PIN_NAME(10, 8), > + RZ_PIN_NAME(10, 9), RZ_PIN_NAME(10, 10), RZ_PIN_NAME(10, 11), > + RZ_PIN_NAME(10, 12), RZ_PIN_NAME(10, 13), RZ_PIN_NAME(10, 14), > + RZ_PIN_NAME(10, 15), > + > + /* Port 10 */ s/Port 10/Port 11/ > + RZ_PIN_NAME(11, 0), RZ_PIN_NAME(11, 1), RZ_PIN_NAME(11, 2), > + RZ_PIN_NAME(11, 3), RZ_PIN_NAME(11, 4), RZ_PIN_NAME(11, 5), > + RZ_PIN_NAME(11, 6), RZ_PIN_NAME(11, 7), RZ_PIN_NAME(11, 8), > + RZ_PIN_NAME(11, 9), RZ_PIN_NAME(11, 10), RZ_PIN_NAME(11, 11), > + RZ_PIN_NAME(11, 12), RZ_PIN_NAME(11, 13), RZ_PIN_NAME(11, 14), > + RZ_PIN_NAME(11, 15), > +}; > + > +struct rz_pin_desc pins[] = { > + /* Port 0 */ > + RZ_PIN_DESC(0, 0), RZ_PIN_DESC(0, 1), RZ_PIN_DESC(0, 2), > + RZ_PIN_DESC(0, 3), RZ_PIN_DESC(0, 4), RZ_PIN_DESC(0, 5), > + > + /* Port 1 */ > + RZ_PIN_DESC(1, 0), RZ_PIN_DESC(1, 1), RZ_PIN_DESC(1, 2), > + RZ_PIN_DESC(1, 3), RZ_PIN_DESC(1, 4), RZ_PIN_DESC(1, 5), > + RZ_PIN_DESC(1, 6), RZ_PIN_DESC(1, 7), RZ_PIN_DESC(1, 8), > + RZ_PIN_DESC(1, 9), RZ_PIN_DESC(1, 10), RZ_PIN_DESC(1, 11), > + RZ_PIN_DESC(1, 12), RZ_PIN_DESC(1, 13), RZ_PIN_DESC(1, 14), > + RZ_PIN_DESC(1, 15), > + > + /* Port 2 */ > + RZ_PIN_DESC(2, 0), RZ_PIN_DESC(2, 1), RZ_PIN_DESC(2, 2), > + RZ_PIN_DESC(2, 3), RZ_PIN_DESC(2, 4), RZ_PIN_DESC(2, 5), > + RZ_PIN_DESC(2, 6), RZ_PIN_DESC(2, 7), RZ_PIN_DESC(2, 8), > + RZ_PIN_DESC(2, 9), RZ_PIN_DESC(2, 10), RZ_PIN_DESC(2, 11), > + RZ_PIN_DESC(2, 12), RZ_PIN_DESC(2, 13), RZ_PIN_DESC(2, 14), > + RZ_PIN_DESC(2, 15), > + > + /* Port 3 */ > + RZ_PIN_DESC(3, 0), RZ_PIN_DESC(3, 1), RZ_PIN_DESC(3, 2), > + RZ_PIN_DESC(3, 3), RZ_PIN_DESC(3, 4), RZ_PIN_DESC(3, 5), > + RZ_PIN_DESC(3, 6), RZ_PIN_DESC(3, 7), RZ_PIN_DESC(3, 8), > + RZ_PIN_DESC(3, 9), RZ_PIN_DESC(3, 10), RZ_PIN_DESC(3, 11), > + RZ_PIN_DESC(3, 12), RZ_PIN_DESC(3, 13), RZ_PIN_DESC(3, 14), > + RZ_PIN_DESC(3, 15), > + > + /* Port 4 */ > + RZ_PIN_DESC(4, 0), RZ_PIN_DESC(4, 1), RZ_PIN_DESC(4, 2), > + RZ_PIN_DESC(4, 3), RZ_PIN_DESC(4, 4), RZ_PIN_DESC(4, 5), > + RZ_PIN_DESC(4, 6), RZ_PIN_DESC(4, 7), RZ_PIN_DESC(4, 8), > + RZ_PIN_DESC(4, 9), RZ_PIN_DESC(4, 10), RZ_PIN_DESC(4, 11), > + RZ_PIN_DESC(4, 12), RZ_PIN_DESC(4, 13), RZ_PIN_DESC(4, 14), > + RZ_PIN_DESC(4, 15), > + > + /* Port 5 */ > + RZ_PIN_DESC(5, 0), RZ_PIN_DESC(5, 1), RZ_PIN_DESC(5, 2), > + RZ_PIN_DESC(5, 3), RZ_PIN_DESC(5, 4), RZ_PIN_DESC(5, 5), > + RZ_PIN_DESC(5, 6), RZ_PIN_DESC(5, 7), RZ_PIN_DESC(5, 8), > + RZ_PIN_DESC(5, 9), RZ_PIN_DESC(5, 10), > + > + /* Port 6 */ > + RZ_PIN_DESC(6, 0), RZ_PIN_DESC(6, 1), RZ_PIN_DESC(6, 2), > + RZ_PIN_DESC(6, 3), RZ_PIN_DESC(6, 4), RZ_PIN_DESC(6, 5), > + RZ_PIN_DESC(6, 6), RZ_PIN_DESC(6, 7), RZ_PIN_DESC(6, 8), > + RZ_PIN_DESC(6, 9), RZ_PIN_DESC(6, 10), RZ_PIN_DESC(6, 11), > + RZ_PIN_DESC(6, 12), RZ_PIN_DESC(6, 13), RZ_PIN_DESC(6, 14), > + RZ_PIN_DESC(6, 15), > + > + /* Port 7 */ > + RZ_PIN_DESC(7, 0), RZ_PIN_DESC(7, 1), RZ_PIN_DESC(7, 2), > + RZ_PIN_DESC(7, 3), RZ_PIN_DESC(7, 4), RZ_PIN_DESC(7, 5), > + RZ_PIN_DESC(7, 6), RZ_PIN_DESC(7, 7), RZ_PIN_DESC(7, 8), > + RZ_PIN_DESC(7, 9), RZ_PIN_DESC(7, 10), RZ_PIN_DESC(7, 11), > + RZ_PIN_DESC(7, 12), RZ_PIN_DESC(7, 13), RZ_PIN_DESC(7, 14), > + RZ_PIN_DESC(7, 15), > + > + /* Port 8 */ > + RZ_PIN_DESC(8, 0), RZ_PIN_DESC(8, 1), RZ_PIN_DESC(8, 2), > + RZ_PIN_DESC(8, 3), RZ_PIN_DESC(8, 4), RZ_PIN_DESC(8, 5), > + RZ_PIN_DESC(8, 6), RZ_PIN_DESC(8, 7), RZ_PIN_DESC(8, 8), > + RZ_PIN_DESC(8, 9), RZ_PIN_DESC(8, 10), RZ_PIN_DESC(8, 11), > + RZ_PIN_DESC(8, 12), RZ_PIN_DESC(8, 13), RZ_PIN_DESC(8, 14), > + RZ_PIN_DESC(8, 15), > + > + /* Port 9 */ > + RZ_PIN_DESC(9, 0), RZ_PIN_DESC(9, 1), RZ_PIN_DESC(9, 2), > + RZ_PIN_DESC(9, 3), RZ_PIN_DESC(9, 4), RZ_PIN_DESC(9, 5), > + RZ_PIN_DESC(9, 6), RZ_PIN_DESC(9, 7), > + > + /* Port 10 */ > + RZ_PIN_DESC(10, 0), RZ_PIN_DESC(10, 1), RZ_PIN_DESC(10, 2), > + RZ_PIN_DESC(10, 3), RZ_PIN_DESC(10, 4), RZ_PIN_DESC(10, 5), > + RZ_PIN_DESC(10, 6), RZ_PIN_DESC(10, 7), RZ_PIN_DESC(10, 8), > + RZ_PIN_DESC(10, 9), RZ_PIN_DESC(10, 10), RZ_PIN_DESC(10, 11), > + RZ_PIN_DESC(10, 12), RZ_PIN_DESC(10, 13), RZ_PIN_DESC(10, 14), > + RZ_PIN_DESC(10, 15), > + > + /* Port 11 */ > + RZ_PIN_DESC(11, 0), RZ_PIN_DESC(11, 1), RZ_PIN_DESC(11, 2), > + RZ_PIN_DESC(11, 3), RZ_PIN_DESC(11, 4), RZ_PIN_DESC(11, 5), > + RZ_PIN_DESC(11, 6), RZ_PIN_DESC(11, 7), RZ_PIN_DESC(11, 8), > + RZ_PIN_DESC(11, 9), RZ_PIN_DESC(11, 10), RZ_PIN_DESC(11, 11), > + RZ_PIN_DESC(11, 12), RZ_PIN_DESC(11, 13), RZ_PIN_DESC(11, 14), > + RZ_PIN_DESC(11, 15), > +}; > + > +/* ------------------------------------------------------------------------ > + * SoC operations > + */ > + > +static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg, > + int bank, int pin, int set) > +{ > + void __iomem *mem = RZA1_ADDR(res->base, reg, bank); > + u16 val = set ? ioread16(mem) | 1 << pin : > + ioread16(mem) & ~(1 << pin); Is the compiler smart enough to avoid two calls to ioread16() ? If not, how about u16 val = ioread16(mem); if (set) val |= 1 << pin; else val &= ~(1 << pin); > +#ifdef RZA1_REG_DBG > + u16 temp = ioread16(mem); > + > + pr_err("%p %p %p - %4x %4x\n", > + (void *)res->start, res->base, mem, temp, val); > +#endif Do you really need this in the driver, or was it useful during initial development only ? > + iowrite16(val, mem); Is there a lock in the call stack that prevents a race condition here ? If not I'd add a spinlock around the read and write. > +} > + > +/** > + * rza1_set_mux() - Configure alternate function settings for the selected > pin > + * > + * @pinctrl: RZ pincontroller > + * @pin: Pin to configure The parameter is named pin_desc. > + * @mux_mode: Alternate function number > + * > + * @return: 0 for success; != 0 otherwise > + */ > +static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl, > + struct rz_pin_desc *pin_desc, unsigned int mux_mode) > +{ > + struct rz_pinctrl_res *res; > + unsigned int bank = pin_desc->bank, > + pin = pin_desc->pin; One declaration per line please, especially when initializing the variables at declaration time. unsigned int bank = pin_desc->bank; unsigned int pin = pin_desc->pin; > + > + /* > + * disable input buffer and bi-control direction before entering > + * alternate mode and let alternate function drive the IO mode by > + * setting PIPCn to 1 Sentences start with a capital letter and end with a period. Or a smiley in e- mails, but usually not in driver code ;-) This comment applies to the whole patch series. > + */ > + res = &pinctrl->res[MEM_RES_HIGH]; > + rza1_set_bit(res, PIBC_REG, bank, pin, 0); > + rza1_set_bit(res, PBDC_REG, bank, pin, 0); > + rza1_set_bit(res, PIPC_REG, bank, pin, 1); > + > + /* TODO: > + * all alternate functions except a few (3) need PIPCn = 1; > + * find a way to identify those 3 functions, do not set PIPCn to 1 > + * and set PMn according to some flag passed as parameter from DTS Nitpicking, I'd say "from DT", DTS is the source format. > + */ > + > + /* > + * enable alternate function mode and select it. > + * > + * ---------------------------------------------------- > + * Alternate mode selection table: > + * > + * PMC PFC PFCE PFCAE AlternateMode mux_mode > + * 1 0 0 0 1 0 > + * 1 1 0 0 2 1 > + * 1 0 1 0 3 2 > + * 1 1 1 0 4 3 > + * 1 0 0 1 5 4 > + * 1 1 0 1 6 5 > + * 1 0 1 1 7 6 > + * 1 1 1 1 8 7 > + * ---------------------------------------------------- > + */ > + res = &pinctrl->res[MEM_RES_LOW]; > + rza1_set_bit(res, PMC_REG, bank, pin, 1); > + rza1_set_bit(res, PFC_REG, bank, pin, mux_mode & 0x1); > + rza1_set_bit(res, PFCE_REG, bank, pin, mux_mode & 0x2); > + rza1_set_bit(res, PFCEA_REG, bank, pin, mux_mode & 0x4); > + > + return 0; > +} > + > +static struct rz_pinctrl_ops rza1_pinctrl_ops = { > + .set_mux = rza1_set_mux, > +}; > + > +static struct rz_pinctrl_info rza1_info = { > + .ops = &rza1_pinctrl_ops, > + > + .npins = ARRAY_SIZE(pins), > + .pins = pins, > +}; You should make those structures static const (which will require updating patch 1/5). It's especially important to make ops structures const for security reasons. > +static const struct of_device_id rza1_pinctrl_of_match[] = { > + { .compatible = "renesas,rza1-pinctrl", }, > + { } > +}; Drivers usually have a corresponding MODULE_DEVICE_TABLE(). As this code can't be compiled as a module I suppose it doesn't matter much. > +static int rza1_pinctrl_probe(struct platform_device *pdev) > +{ > + return rz_pinctrl_probe(pdev, &rza1_info); > +} > + > +static int rza1_pinctrl_remove(struct platform_device *pdev) > +{ > + rz_pinctrl_remove(pdev); > + > + return 0; > +} > + > +static struct platform_driver rza1_pinctrl_driver = { > + .driver = { > + .name = "rza1_pinctrl_driver", No need for "_driver" here, "rza1_pinctrl" will do. I'm also never sure whether we should use _ or - in driver names. > + .of_match_table = rza1_pinctrl_of_match, > + }, > + .probe = rza1_pinctrl_probe, > + .remove = rza1_pinctrl_remove, > +}; > + > +static int __init rza1_pinctrl_init(void) > +{ > + return platform_driver_register(&rza1_pinctrl_driver); > +} > +core_initcall(rza1_pinctrl_init); > + > +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org"); > +MODULE_DESCRIPTION("Pinctl driver for Reneas RZ/A1 SoC"); s/Pinctl/Pinctrl/ (or better, "Pin controller" ?) > +MODULE_LICENSE("GPL v2"); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi 2017-01-25 18:09 ` [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module Jacopo Mondi 2017-01-25 18:09 ` [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver Jacopo Mondi @ 2017-01-25 18:09 ` Jacopo Mondi 2017-01-26 19:52 ` Geert Uytterhoeven 2017-01-25 18:09 ` [RFC 4/5] arm: dts: r7s1000: Add pincontroller node Jacopo Mondi ` (5 subsequent siblings) 8 siblings, 1 reply; 54+ messages in thread From: Jacopo Mondi @ 2017-01-25 18:09 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Add dt-bindings header for Renesas RZ pincontroller. The header defines macros for pin description and alternate function numbers. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h diff --git a/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h new file mode 100644 index 0000000..92816d4 --- /dev/null +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h @@ -0,0 +1,19 @@ +/* + * Defines macros and constants for Renesas RZ pin controller and muxer + */ + +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZ_H +#define __DT_BINDINGS_PINCTRL_RENESAS_RZ_H + +#define RZ_PIN(b, p) b p + +#define ALTERNATE_FUNC_1 0 +#define ALTERNATE_FUNC_2 1 +#define ALTERNATE_FUNC_3 2 +#define ALTERNATE_FUNC_4 3 +#define ALTERNATE_FUNC_5 4 +#define ALTERNATE_FUNC_6 5 +#define ALTERNATE_FUNC_7 6 +#define ALTERNATE_FUNC_8 7 + +#endif -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header 2017-01-25 18:09 ` [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header Jacopo Mondi @ 2017-01-26 19:52 ` Geert Uytterhoeven 2017-01-30 18:30 ` Laurent Pinchart 0 siblings, 1 reply; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-26 19:52 UTC (permalink / raw) To: Jacopo Mondi Cc: Laurent Pinchart, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Jacopo, On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > Add dt-bindings header for Renesas RZ pincontroller. > The header defines macros for pin description and alternate function > numbers. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h > > diff --git a/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h > new file mode 100644 > index 0000000..92816d4 > --- /dev/null > +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h > @@ -0,0 +1,19 @@ > +/* > + * Defines macros and constants for Renesas RZ pin controller and muxer > + */ > + > +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZ_H > +#define __DT_BINDINGS_PINCTRL_RENESAS_RZ_H > + > +#define RZ_PIN(b, p) b p And the advantage of this macro is? > +#define ALTERNATE_FUNC_1 0 > +#define ALTERNATE_FUNC_2 1 > +#define ALTERNATE_FUNC_3 2 > +#define ALTERNATE_FUNC_4 3 > +#define ALTERNATE_FUNC_5 4 > +#define ALTERNATE_FUNC_6 5 > +#define ALTERNATE_FUNC_7 6 > +#define ALTERNATE_FUNC_8 7 I have mixed feelings about these macros: 1. They're long to type, 2. They just map from n to n-1. Why not use plain numbers 1..8 (the alternate function numbering in the datasheet is 1-based), and subtract 1 in the C code? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header 2017-01-26 19:52 ` Geert Uytterhoeven @ 2017-01-30 18:30 ` Laurent Pinchart 2017-01-31 9:12 ` jacopo mondi 0 siblings, 1 reply; 54+ messages in thread From: Laurent Pinchart @ 2017-01-30 18:30 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Jacopo Mondi, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio On Thursday 26 Jan 2017 20:52:33 Geert Uytterhoeven wrote: > On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi wrote: > > Add dt-bindings header for Renesas RZ pincontroller. > > The header defines macros for pin description and alternate function > > numbers. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > > > include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h > > > > diff --git a/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h > > b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h new file mode 100644 > > index 0000000..92816d4 > > --- /dev/null > > +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h > > @@ -0,0 +1,19 @@ > > +/* > > + * Defines macros and constants for Renesas RZ pin controller and muxer > > + */ > > + > > +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZ_H > > +#define __DT_BINDINGS_PINCTRL_RENESAS_RZ_H > > + > > +#define RZ_PIN(b, p) b p > > And the advantage of this macro is? > > > +#define ALTERNATE_FUNC_1 0 > > +#define ALTERNATE_FUNC_2 1 > > +#define ALTERNATE_FUNC_3 2 > > +#define ALTERNATE_FUNC_4 3 > > +#define ALTERNATE_FUNC_5 4 > > +#define ALTERNATE_FUNC_6 5 > > +#define ALTERNATE_FUNC_7 6 > > +#define ALTERNATE_FUNC_8 7 > > I have mixed feelings about these macros: > 1. They're long to type, > 2. They just map from n to n-1. > > Why not use plain numbers 1..8 (the alternate function numbering in the > datasheet is 1-based), and subtract 1 in the C code? I was about to mention the same. I think you can drop this patch and use the numbers directly. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header 2017-01-30 18:30 ` Laurent Pinchart @ 2017-01-31 9:12 ` jacopo mondi 2017-01-31 9:31 ` Geert Uytterhoeven 0 siblings, 1 reply; 54+ messages in thread From: jacopo mondi @ 2017-01-31 9:12 UTC (permalink / raw) To: Laurent Pinchart, Geert Uytterhoeven Cc: Jacopo Mondi, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Geert, Laurent, On 30/01/2017 19:30, Laurent Pinchart wrote: > On Thursday 26 Jan 2017 20:52:33 Geert Uytterhoeven wrote: >> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi wrote: >>> Add dt-bindings header for Renesas RZ pincontroller. >>> The header defines macros for pin description and alternate function >>> numbers. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> >>> include/dt-bindings/pinctrl/pinctrl-renesas-rz.h | 19 +++++++++++++++++++ >>> 1 file changed, 19 insertions(+) >>> create mode 100644 include/dt-bindings/pinctrl/pinctrl-renesas-rz.h >>> >>> diff --git a/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h >>> b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h new file mode 100644 >>> index 0000000..92816d4 >>> --- /dev/null >>> +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h >>> @@ -0,0 +1,19 @@ >>> +/* >>> + * Defines macros and constants for Renesas RZ pin controller and muxer >>> + */ >>> + >>> +#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZ_H >>> +#define __DT_BINDINGS_PINCTRL_RENESAS_RZ_H >>> + >>> +#define RZ_PIN(b, p) b p >> >> And the advantage of this macro is? >> Nothing beside being cute >>> +#define ALTERNATE_FUNC_1 0 >>> +#define ALTERNATE_FUNC_2 1 >>> +#define ALTERNATE_FUNC_3 2 >>> +#define ALTERNATE_FUNC_4 3 >>> +#define ALTERNATE_FUNC_5 4 >>> +#define ALTERNATE_FUNC_6 5 >>> +#define ALTERNATE_FUNC_7 6 >>> +#define ALTERNATE_FUNC_8 7 >> >> I have mixed feelings about these macros: >> 1. They're long to type, >> 2. They just map from n to n-1. >> >> Why not use plain numbers 1..8 (the alternate function numbering in the >> datasheet is 1-based), and subtract 1 in the C code? > > I was about to mention the same. I think you can drop this patch and use the > numbers directly. > Please be aware there are values we'll have to define here, such as the additional configurations we're talking about in some other part of this email thread. This may become something like: #define ALT_FUNC_1 0 #define ALT_FUNC_2 1 #define ALT_FUNC_3 2 #define ALT_FUNC_4 3 #define ALT_FUNC_5 4 #define ALT_FUNC_6 5 #define ALT_FUNC_7 6 #define ALT_FUNC_8 7 #define INPUT_MODE 1 #define BIDRECTIONAL 2 #define ... #define PIN(b, p) b p #define PIN_MUX(func, conf) \ ((func & 0xf) | ((conf) << 16)) and in DTS sources you would describe a pin as pins = <PIN(bank, pin) PIN_MUX(ALT_FUNC_1, INPUT | BIDIRECTIONAL)>; We can drop the PIN macro as it does not bring anything I agree, but it's nicer to see imho Thanks j ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header 2017-01-31 9:12 ` jacopo mondi @ 2017-01-31 9:31 ` Geert Uytterhoeven 2017-01-31 14:00 ` Chris Brandt 0 siblings, 1 reply; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-31 9:31 UTC (permalink / raw) To: jacopo mondi Cc: Laurent Pinchart, Jacopo Mondi, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Jacopo, On Tue, Jan 31, 2017 at 10:12 AM, jacopo mondi <jacopo@jmondi.org> wrote: > On 30/01/2017 19:30, Laurent Pinchart wrote: >> On Thursday 26 Jan 2017 20:52:33 Geert Uytterhoeven wrote: >>> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi wrote: >>>> Add dt-bindings header for Renesas RZ pincontroller. >>>> The header defines macros for pin description and alternate function >>>> numbers. >>>> --- /dev/null >>>> +++ b/include/dt-bindings/pinctrl/pinctrl-renesas-rz.h >>>> +#define RZ_PIN(b, p) b p >>> >>> And the advantage of this macro is? >>> > > Nothing beside being cute :-) >>>> +#define ALTERNATE_FUNC_1 0 >>>> +#define ALTERNATE_FUNC_2 1 >>>> +#define ALTERNATE_FUNC_3 2 >>>> +#define ALTERNATE_FUNC_4 3 >>>> +#define ALTERNATE_FUNC_5 4 >>>> +#define ALTERNATE_FUNC_6 5 >>>> +#define ALTERNATE_FUNC_7 6 >>>> +#define ALTERNATE_FUNC_8 7 >>> >>> I have mixed feelings about these macros: >>> 1. They're long to type, >>> 2. They just map from n to n-1. >>> >>> Why not use plain numbers 1..8 (the alternate function numbering in the >>> datasheet is 1-based), and subtract 1 in the C code? >> >> I was about to mention the same. I think you can drop this patch and use >> the >> numbers directly. > > Please be aware there are values we'll have to define here, such as the > additional configurations we're talking about in some other part of this > email thread. In that case you indeed have to OR values, but... > This may become something like: > > #define ALT_FUNC_1 0 > #define ALT_FUNC_2 1 > #define ALT_FUNC_3 2 > #define ALT_FUNC_4 3 > #define ALT_FUNC_5 4 > #define ALT_FUNC_6 5 > #define ALT_FUNC_7 6 > #define ALT_FUNC_8 7 > > #define INPUT_MODE 1 > #define BIDRECTIONAL 2 > #define ... ... if you integrate the shifts in the definitions above, e.g. #define INPUT_MODE (1 << 16) > #define PIN(b, p) b p > #define PIN_MUX(func, conf) \ > ((func & 0xf) | ((conf) << 16)) > > and in DTS sources you would describe a pin as > > pins = <PIN(bank, pin) PIN_MUX(ALT_FUNC_1, INPUT | BIDIRECTIONAL)>; > > We can drop the PIN macro as it does not bring anything I agree, but it's > nicer to see imho ... you can keep plain 1-based numbers for alternate functions, and use the definitions for flags, e.g.: pins = <PIN(bank, pin) (1 | INPUT_MODE | BIDIRECTIONAL)>; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header 2017-01-31 9:31 ` Geert Uytterhoeven @ 2017-01-31 14:00 ` Chris Brandt 0 siblings, 0 replies; 54+ messages in thread From: Chris Brandt @ 2017-01-31 14:00 UTC (permalink / raw) To: Geert Uytterhoeven, jacopo mondi Cc: Laurent Pinchart, Jacopo Mondi, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio On Tuesday, January 31, 2017, Geert Uytterhoeven wrote: > > This may become something like: > > > > #define ALT_FUNC_1 0 > > #define ALT_FUNC_2 1 > > #define ALT_FUNC_3 2 > > #define ALT_FUNC_4 3 > > #define ALT_FUNC_5 4 > > #define ALT_FUNC_6 5 > > #define ALT_FUNC_7 6 > > #define ALT_FUNC_8 7 ALT_FUNC_1 is still too long if you're going to OR things on 1 line in the DT. I personally like: #define ALT_1 0 > > #define PIN(b, p) b p > > #define PIN_MUX(func, conf) \ > > ((func & 0xf) | ((conf) << 16)) > > > > and in DTS sources you would describe a pin as > > > > pins = <PIN(bank, pin) PIN_MUX(ALT_FUNC_1, INPUT | BIDIRECTIONAL)>; > > > > We can drop the PIN macro as it does not bring anything I agree, but > > it's nicer to see imho > > ... you can keep plain 1-based numbers for alternate functions, and use > the definitions for flags, e.g.: > > pins = <PIN(bank, pin) (1 | INPUT_MODE | BIDIRECTIONAL)>; To Geert's point, nothing is going to be "shorter" than just using '1','2','3',etc.. I also think "BIDIRECTIONAL" >> "BIDIR" Or....we could make life easy on people and #define the pins that are tricky: #define I2C BIDIRECTIONAL #define SDHI_DATA BIDIRECTIONAL #define SDHI_CTRL /* none */ #define RSPI /* none */ #define ETHERNET /* none */ Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi ` (2 preceding siblings ...) 2017-01-25 18:09 ` [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header Jacopo Mondi @ 2017-01-25 18:09 ` Jacopo Mondi 2017-01-26 9:49 ` Sergei Shtylyov ` (2 more replies) 2017-01-25 18:09 ` [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group Jacopo Mondi ` (4 subsequent siblings) 8 siblings, 3 replies; 54+ messages in thread From: Jacopo Mondi @ 2017-01-25 18:09 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Add pincontroller node compatible with the new Renesas RZ/A1 pincontroller driver. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi index 3dd427d..764006d 100644 --- a/arch/arm/boot/dts/r7s72100.dtsi +++ b/arch/arm/boot/dts/r7s72100.dtsi @@ -171,6 +171,18 @@ }; }; + pinctrl: pinctrl@fcfe3000 { + compatible = "renesas,rza1-pinctrl"; + #address-cells = <1>; + #size-cells = <0>; + + #pinctrl-cells = <2>; + + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ + <0xfcfe7B00 0x430>; /* JPPR0, ..., JPIBC0 */ + }; + scif0: serial@e8007000 { compatible = "renesas,scif-r7s72100", "renesas,scif"; reg = <0xe8007000 64>; -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-25 18:09 ` [RFC 4/5] arm: dts: r7s1000: Add pincontroller node Jacopo Mondi @ 2017-01-26 9:49 ` Sergei Shtylyov 2017-01-26 19:54 ` Geert Uytterhoeven 2017-01-30 18:29 ` Laurent Pinchart 2 siblings, 0 replies; 54+ messages in thread From: Sergei Shtylyov @ 2017-01-26 9:49 UTC (permalink / raw) To: Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hello! s/r7s1000/r7s72100/ in the subject. On 1/25/2017 9:09 PM, Jacopo Mondi wrote: > Add pincontroller node compatible with the new Renesas RZ/A1 > pincontroller driver. Pin controller. > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi > index 3dd427d..764006d 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -171,6 +171,18 @@ > }; > }; > > + pinctrl: pinctrl@fcfe3000 { I'd call the node "pin-controller@...". MBR, Sergei ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-25 18:09 ` [RFC 4/5] arm: dts: r7s1000: Add pincontroller node Jacopo Mondi 2017-01-26 9:49 ` Sergei Shtylyov @ 2017-01-26 19:54 ` Geert Uytterhoeven 2017-01-31 10:24 ` jacopo mondi 2017-01-30 18:29 ` Laurent Pinchart 2 siblings, 1 reply; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-26 19:54 UTC (permalink / raw) To: Jacopo Mondi Cc: Laurent Pinchart, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Jacopo, On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > Add pincontroller node compatible with the new Renesas RZ/A1 > pincontroller driver. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi > index 3dd427d..764006d 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -171,6 +171,18 @@ > }; > }; > > + pinctrl: pinctrl@fcfe3000 { > + compatible = "renesas,rza1-pinctrl"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + #pinctrl-cells = <2>; Souldn't that be <3>? E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-26 19:54 ` Geert Uytterhoeven @ 2017-01-31 10:24 ` jacopo mondi 2017-01-31 10:34 ` Geert Uytterhoeven 0 siblings, 1 reply; 54+ messages in thread From: jacopo mondi @ 2017-01-31 10:24 UTC (permalink / raw) To: Geert Uytterhoeven, Jacopo Mondi Cc: Laurent Pinchart, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Geert, On 26/01/2017 20:54, Geert Uytterhoeven wrote: > Hi Jacopo, > > On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: >> Add pincontroller node compatible with the new Renesas RZ/A1 >> pincontroller driver. >> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> --- >> arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi >> index 3dd427d..764006d 100644 >> --- a/arch/arm/boot/dts/r7s72100.dtsi >> +++ b/arch/arm/boot/dts/r7s72100.dtsi >> @@ -171,6 +171,18 @@ >> }; >> }; >> >> + pinctrl: pinctrl@fcfe3000 { >> + compatible = "renesas,rza1-pinctrl"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + #pinctrl-cells = <2>; > > Souldn't that be <3>? > E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers. > From pinctrl-bindings.txt: #pinctrl-cells: Number of pin control cells in addition to the index within the pin controller device instance So here it's (2 + 1) as at least the pin index (first parameter) is mandatory. We're twisting the assumption of having "index" as first, single, parameter, as we have <bank pin mode> and the <bank, pin> pair identifies a pin. Hope this is ok, and we can re-use existing bindings even if our semantic is a bit different. > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-31 10:24 ` jacopo mondi @ 2017-01-31 10:34 ` Geert Uytterhoeven 0 siblings, 0 replies; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-31 10:34 UTC (permalink / raw) To: jacopo mondi Cc: Jacopo Mondi, Laurent Pinchart, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Jacopo, On Tue, Jan 31, 2017 at 11:24 AM, jacopo mondi <jacopo@jmondi.org> wrote: > On 26/01/2017 20:54, Geert Uytterhoeven wrote: >> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> >> wrote: >>> >>> Add pincontroller node compatible with the new Renesas RZ/A1 >>> pincontroller driver. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/r7s72100.dtsi >>> b/arch/arm/boot/dts/r7s72100.dtsi >>> index 3dd427d..764006d 100644 >>> --- a/arch/arm/boot/dts/r7s72100.dtsi >>> +++ b/arch/arm/boot/dts/r7s72100.dtsi >>> @@ -171,6 +171,18 @@ >>> }; >>> }; >>> >>> + pinctrl: pinctrl@fcfe3000 { >>> + compatible = "renesas,rza1-pinctrl"; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + #pinctrl-cells = <2>; >> >> >> Souldn't that be <3>? >> E.g. <RZ_PIN(3, 0) ALTERNATE_FUNC_6> expands to <3 0 5>, i.e. 3 numbers. >> > > From pinctrl-bindings.txt: > > #pinctrl-cells: Number of pin control cells in addition to the index within > the pin controller device instance IC. I incorrectly assumed it would be the number of cells after a pinctrl phandle, which is superfluous here as these are subnodes of the pfc node. > So here it's (2 + 1) as at least the pin index (first parameter) is > mandatory. > > We're twisting the assumption of having "index" as first, single, parameter, > as we have <bank pin mode> and the <bank, pin> pair identifies a pin. > > Hope this is ok, and we can re-use existing bindings even if our semantic is > a bit different. That's indeed a bit of twisting ;-) You can fix that by keeping the RZ_PIN() macro, and changing it to e.g. #define RZ_PIN(bank, pin) ((bank) << 16 | (pin)) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-25 18:09 ` [RFC 4/5] arm: dts: r7s1000: Add pincontroller node Jacopo Mondi 2017-01-26 9:49 ` Sergei Shtylyov 2017-01-26 19:54 ` Geert Uytterhoeven @ 2017-01-30 18:29 ` Laurent Pinchart 2017-01-30 19:39 ` Chris Brandt 2 siblings, 1 reply; 54+ messages in thread From: Laurent Pinchart @ 2017-01-30 18:29 UTC (permalink / raw) To: Jacopo Mondi; +Cc: geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Jacopo, Thank you for the patch. On Wednesday 25 Jan 2017 19:09:46 Jacopo Mondi wrote: > Add pincontroller node compatible with the new Renesas RZ/A1 > pincontroller driver. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi > b/arch/arm/boot/dts/r7s72100.dtsi index 3dd427d..764006d 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -171,6 +171,18 @@ > }; > }; > > + pinctrl: pinctrl@fcfe3000 { > + compatible = "renesas,rza1-pinctrl"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + #pinctrl-cells = <2>; > + > + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ > + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ What's the reason for splitting those registers in two sets ? Maybe you can explain that in the DT bindings documentation that this patch series is missing ;-) > + <0xfcfe7B00 0x430>; /* JPPR0, ..., JPIBC0 */ s/B/b/ > + }; > + > scif0: serial@e8007000 { > compatible = "renesas,scif-r7s72100", "renesas,scif"; > reg = <0xe8007000 64>; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-30 18:29 ` Laurent Pinchart @ 2017-01-30 19:39 ` Chris Brandt 2017-01-31 10:35 ` Laurent Pinchart 0 siblings, 1 reply; 54+ messages in thread From: Chris Brandt @ 2017-01-30 19:39 UTC (permalink / raw) To: Laurent Pinchart, Jacopo Mondi Cc: geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Jacopo, On Monday, January 30, 2017, Laurent Pinchart wrote: > > + pinctrl: pinctrl@fcfe3000 { > > + compatible = "renesas,rza1-pinctrl"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + #pinctrl-cells = <2>; > > + > > + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ > > + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ > > What's the reason for splitting those registers in two sets ? Maybe you > can explain that in the DT bindings documentation that this patch series > is missing ;-) I left this out of my review comments, but even though the chip designers left a BIG HOLE in the memory map of the PFC controller, I don't think it will 'cost' you anything by just mapping the whole area (dead space and all) and getting rid of the "high and low" memory indexing thing that you are doing in the driver. There is nothing mapped in that dead area anyway. Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-30 19:39 ` Chris Brandt @ 2017-01-31 10:35 ` Laurent Pinchart 2017-01-31 14:08 ` Chris Brandt 0 siblings, 1 reply; 54+ messages in thread From: Laurent Pinchart @ 2017-01-31 10:35 UTC (permalink / raw) To: Chris Brandt Cc: Jacopo Mondi, geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Chris, On Monday 30 Jan 2017 19:39:33 Chris Brandt wrote: > On Monday, January 30, 2017, Laurent Pinchart wrote: > >> + pinctrl: pinctrl@fcfe3000 { > >> + compatible = "renesas,rza1-pinctrl"; > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + #pinctrl-cells = <2>; > >> + > >> + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ > >> + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ > > > > What's the reason for splitting those registers in two sets ? Maybe you > > can explain that in the DT bindings documentation that this patch series > > is missing ;-) > > I left this out of my review comments, but even though the chip designers > left a BIG HOLE in the memory map of the PFC controller, I don't think it > will 'cost' you anything by just mapping the whole area (dead space and > all) and getting rid of the "high and low" memory indexing thing that you > are doing in the driver. > There is nothing mapped in that dead area anyway. For the first two areas, I agree. The third area is a separate pin controller for the JTAG port, not multiplexed with the GPIO ports. I even wonder whether it should be split in a separate DT node. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 4/5] arm: dts: r7s1000: Add pincontroller node 2017-01-31 10:35 ` Laurent Pinchart @ 2017-01-31 14:08 ` Chris Brandt 0 siblings, 0 replies; 54+ messages in thread From: Chris Brandt @ 2017-01-31 14:08 UTC (permalink / raw) To: Laurent Pinchart, Jacopo Mondi Cc: geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hi Laurent (and Jacopo) On Tuesday, January 31, 2017, Laurent Pinchart wrote: > On Monday 30 Jan 2017 19:39:33 Chris Brandt wrote: > > On Monday, January 30, 2017, Laurent Pinchart wrote: > > >> + pinctrl: pinctrl@fcfe3000 { > > >> + compatible = "renesas,rza1-pinctrl"; > > >> + #address-cells = <1>; > > >> + #size-cells = <0>; > > >> + > > >> + #pinctrl-cells = <2>; > > >> + > > >> + reg = <0xfcfe3000 0xa30>, /* Pn, ..., PFCAEn */ > > >> + <0xfcfe7000 0x230>, /* PIBCn, ..., PIPCn */ > > > > > > What's the reason for splitting those registers in two sets ? Maybe > > > you can explain that in the DT bindings documentation that this > > > patch series is missing ;-) > > > > I left this out of my review comments, but even though the chip > > designers left a BIG HOLE in the memory map of the PFC controller, I > > don't think it will 'cost' you anything by just mapping the whole area > > (dead space and > > all) and getting rid of the "high and low" memory indexing thing that > > you are doing in the driver. > > There is nothing mapped in that dead area anyway. > > For the first two areas, I agree. The third area is a separate pin > controller for the JTAG port, not multiplexed with the GPIO ports. I even > wonder whether it should be split in a separate DT node. I think we should just forget about the JTAG pins completely. Honestly, they are 2 pins that can only be configured as JTAG or a GPIO input (not even output). No one is using those pins for anything other than JTAG. IMHO, adding extra support just for those 2 pins is basically a waste of code. Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi ` (3 preceding siblings ...) 2017-01-25 18:09 ` [RFC 4/5] arm: dts: r7s1000: Add pincontroller node Jacopo Mondi @ 2017-01-25 18:09 ` Jacopo Mondi 2017-01-26 9:51 ` Sergei Shtylyov 2017-01-26 2:57 ` [RFC 0/5] Renesas RZ series pinctrl driver Chris Brandt ` (3 subsequent siblings) 8 siblings, 1 reply; 54+ messages in thread From: Jacopo Mondi @ 2017-01-25 18:09 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Add TxD and RxD pin configuration for SCIF2 serial communication interface on r7s72100 Genmai board. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- arch/arm/boot/dts/r7s72100-genmai.dts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts index 118a8e2..ea649c7 100644 --- a/arch/arm/boot/dts/r7s72100-genmai.dts +++ b/arch/arm/boot/dts/r7s72100-genmai.dts @@ -11,6 +11,7 @@ /dts-v1/; #include "r7s72100.dtsi" +#include "include/dt-bindings/pinctrl/pinctrl-renesas-rz.h" / { model = "Genmai"; @@ -34,6 +35,18 @@ #address-cells = <1>; #size-cells = <1>; }; + +}; + +&pinctrl { + pinctrl-names = "default"; + pinctrl-0 = <&scif2_pins>; + + scif2_pins: serial2 { + /* P3_0 as TxD2; P3_2 as RxD2 */ + renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>, + <RZ_PIN(3, 2) ALTERNATE_FUNC_4>; + }; }; &extal_clk { -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group 2017-01-25 18:09 ` [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group Jacopo Mondi @ 2017-01-26 9:51 ` Sergei Shtylyov 2017-01-30 18:17 ` Laurent Pinchart 0 siblings, 1 reply; 54+ messages in thread From: Sergei Shtylyov @ 2017-01-26 9:51 UTC (permalink / raw) To: Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio On 1/25/2017 9:09 PM, Jacopo Mondi wrote: > Add TxD and RxD pin configuration for SCIF2 serial communication > interface on r7s72100 Genmai board. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > arch/arm/boot/dts/r7s72100-genmai.dts | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts > index 118a8e2..ea649c7 100644 > --- a/arch/arm/boot/dts/r7s72100-genmai.dts > +++ b/arch/arm/boot/dts/r7s72100-genmai.dts [...] > @@ -34,6 +35,18 @@ > #address-cells = <1>; > #size-cells = <1>; > }; > + > +}; > + > +&pinctrl { > + pinctrl-names = "default"; > + pinctrl-0 = <&scif2_pins>; > + > + scif2_pins: serial2 { > + /* P3_0 as TxD2; P3_2 as RxD2 */ > + renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>, The comma should be after the vendor prefix ("renesas"). MBR, Sergei ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group 2017-01-26 9:51 ` Sergei Shtylyov @ 2017-01-30 18:17 ` Laurent Pinchart 2017-01-30 18:55 ` Geert Uytterhoeven 0 siblings, 1 reply; 54+ messages in thread From: Laurent Pinchart @ 2017-01-30 18:17 UTC (permalink / raw) To: Sergei Shtylyov Cc: Jacopo Mondi, geert+renesas, linus.walleij, linux-renesas-soc, linux-gpio Hello, On Thursday 26 Jan 2017 12:51:03 Sergei Shtylyov wrote: > On 1/25/2017 9:09 PM, Jacopo Mondi wrote: > > Add TxD and RxD pin configuration for SCIF2 serial communication > > interface on r7s72100 Genmai board. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > > > arch/arm/boot/dts/r7s72100-genmai.dts | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts > > b/arch/arm/boot/dts/r7s72100-genmai.dts index 118a8e2..ea649c7 100644 > > --- a/arch/arm/boot/dts/r7s72100-genmai.dts > > +++ b/arch/arm/boot/dts/r7s72100-genmai.dts > > [...] > > > @@ -34,6 +35,18 @@ > > #address-cells = <1>; > > #size-cells = <1>; > > }; > > + That's not needed. > > +}; > > + > > +&pinctrl { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&scif2_pins>; > > + > > + scif2_pins: serial2 { > > + /* P3_0 as TxD2; P3_2 as RxD2 */ > > + renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>, > > The comma should be after the vendor prefix ("renesas"). And the "-rz" suffix isn't needed, "renesas,pins" will do. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group 2017-01-30 18:17 ` Laurent Pinchart @ 2017-01-30 18:55 ` Geert Uytterhoeven 0 siblings, 0 replies; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-30 18:55 UTC (permalink / raw) To: Laurent Pinchart Cc: Sergei Shtylyov, Jacopo Mondi, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Laurent, On Mon, Jan 30, 2017 at 7:17 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thursday 26 Jan 2017 12:51:03 Sergei Shtylyov wrote: >> On 1/25/2017 9:09 PM, Jacopo Mondi wrote: >> > +&pinctrl { >> > + pinctrl-names = "default"; >> > + pinctrl-0 = <&scif2_pins>; >> > + >> > + scif2_pins: serial2 { >> > + /* P3_0 as TxD2; P3_2 as RxD2 */ >> > + renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>, >> >> The comma should be after the vendor prefix ("renesas"). > > And the "-rz" suffix isn't needed, "renesas,pins" will do. Then it can just become "pins"? Or not? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 0/5] Renesas RZ series pinctrl driver 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi ` (4 preceding siblings ...) 2017-01-25 18:09 ` [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group Jacopo Mondi @ 2017-01-26 2:57 ` Chris Brandt 2017-01-26 19:56 ` Geert Uytterhoeven ` (2 subsequent siblings) 8 siblings, 0 replies; 54+ messages in thread From: Chris Brandt @ 2017-01-26 2:57 UTC (permalink / raw) To: Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij Cc: linux-renesas-soc, linux-gpio Hi Jacopo, Thanks for the patches. On Wednesday, January 25, 2017, Jacopo Mondi wrote: > Right now, the only "SoC" module support implemented is for RZ/A1H (Genmai > and GR-Peach boards). I'm going to give it a try on the RZ/A1 RSK board. > I have tested the correctness of mux settings printing out register values, > and enabling/disabling the SCIF2 module connected to serial debug > interface. I can give it a try on things like Eth, I2C, SPI, SDHI, MMC. > One note on the current DT ABI: > right now a pin configuration is specified in DTS using utility macros > defined in the (currently undocumented) arch/arm/boot/dts/include/dt- > bindings/pinctrl/pinctrl-renesas-rz.h header file. > Each pin configuration is a triplet of u32 in the form of > > <BANK PIN ALTERNATE_FUNC_#> > > It should be fairly easy adding additional parameters to configure what > was missing in the original group-based PFC driver for RZ devices (I'm > thinking of IO mode control, input buffer configuration, bi-directional > configuration etc). SDHI is one that will need another parameter. Half the pins need Bidirection (PBDC) enabled, and the other half need PBDC disabled. SDHI data pins are PBDC=1, the others are PBDC=0. > The series makes use of newly introduced pin[ctrl|mux]_generic functions, > currently only available in Linus Walleij's linux-pinctrl.git tree. Hmmm, I was hoping to 'easily' back port this to 4.9. I guess if it's the best way forward, then maybe I can back port those new generic functions for the LTSI tree. Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 0/5] Renesas RZ series pinctrl driver 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi ` (5 preceding siblings ...) 2017-01-26 2:57 ` [RFC 0/5] Renesas RZ series pinctrl driver Chris Brandt @ 2017-01-26 19:56 ` Geert Uytterhoeven 2017-01-27 16:47 ` [RFC fixes 0/2] FIX: " Jacopo Mondi 2017-01-30 13:51 ` [RFC 0/5] " Linus Walleij 8 siblings, 0 replies; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-26 19:56 UTC (permalink / raw) To: Jacopo Mondi Cc: Laurent Pinchart, Geert Uytterhoeven, Linus Walleij, Linux-Renesas, linux-gpio Hi Jacopo, On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > after having discussed in great detail the RZ series per-pin PFC hardware > peculiarities, this is a proposal for a possible pin-based pin controller > driver for SoC devices of Renesas RZ family. "RZ" is a bad name, as there are (currently) 3 RZ subfamilies, and they differ a lot. So it's better to use "RZ/A". Or perhaps even "RZ/A1", as you never know what's going to happen with "2"... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi ` (6 preceding siblings ...) 2017-01-26 19:56 ` Geert Uytterhoeven @ 2017-01-27 16:47 ` Jacopo Mondi 2017-01-27 16:47 ` [RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins Jacopo Mondi ` (2 more replies) 2017-01-30 13:51 ` [RFC 0/5] " Linus Walleij 8 siblings, 3 replies; 54+ messages in thread From: Jacopo Mondi @ 2017-01-27 16:47 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij, Chris.Brandt, wsa Cc: linux-renesas-soc, linux-gpio Hello, sorry if I'm sending 2 patches on top of an RFC series with comments still pending, but these patches enabled me to properly test pin configuration sequence in order to access the internal EEPROM through RIIC2 interface on pins 1_4 and 1_5. The outcome is a bugfix to RZ/A1 pincontroller driver which [2/2] applies on. When sending v2 of the whole series I'll probably squash these, but if someone is testing the RFC series I wanted to make sure he does not waste his time with a broken driver. Thanks j Jacopo Mondi (2): arm: dts: genmai: Configure RIIC2 pins pinctrl: rz-pfc: Fix RZ/A1 pin function configuration arch/arm/boot/dts/r7s72100-genmai.dts | 8 ++++- drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 55 +++++++++++++++++++++++------------ 2 files changed, 43 insertions(+), 20 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins 2017-01-27 16:47 ` [RFC fixes 0/2] FIX: " Jacopo Mondi @ 2017-01-27 16:47 ` Jacopo Mondi 2017-01-30 18:49 ` Laurent Pinchart 2017-01-27 16:47 ` [RFC fixes 2/2] pinctrl: rz-pfc: Fix RZ/A1 pin function configuration Jacopo Mondi 2017-01-27 21:09 ` [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver Chris Brandt 2 siblings, 1 reply; 54+ messages in thread From: Jacopo Mondi @ 2017-01-27 16:47 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij, Chris.Brandt, wsa Cc: linux-renesas-soc, linux-gpio Add pin configuration for RIIC2 pins interface. The i2c2 is connected to internal eeprom. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- arch/arm/boot/dts/r7s72100-genmai.dts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts index ea649c7..0068a1a 100644 --- a/arch/arm/boot/dts/r7s72100-genmai.dts +++ b/arch/arm/boot/dts/r7s72100-genmai.dts @@ -40,13 +40,19 @@ &pinctrl { pinctrl-names = "default"; - pinctrl-0 = <&scif2_pins>; + pinctrl-0 = <&scif2_pins &i2c2_pins>; scif2_pins: serial2 { /* P3_0 as TxD2; P3_2 as RxD2 */ renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>, <RZ_PIN(3, 2) ALTERNATE_FUNC_4>; }; + + i2c2_pins: i2c2 { + /* RIIC2: P1_4 as SCL, P1_5 as SDA */ + renesas-rz,pins = <RZ_PIN(1, 4) ALTERNATE_FUNC_1>, + <RZ_PIN(1, 5) ALTERNATE_FUNC_1>; + }; }; &extal_clk { -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins 2017-01-27 16:47 ` [RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins Jacopo Mondi @ 2017-01-30 18:49 ` Laurent Pinchart 0 siblings, 0 replies; 54+ messages in thread From: Laurent Pinchart @ 2017-01-30 18:49 UTC (permalink / raw) To: Jacopo Mondi Cc: geert+renesas, linus.walleij, Chris.Brandt, wsa, linux-renesas-soc, linux-gpio Hi Jacopo, Thank you for the patch. On Friday 27 Jan 2017 17:47:07 Jacopo Mondi wrote: > Add pin configuration for RIIC2 pins interface. > The i2c2 is connected to internal eeprom. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > arch/arm/boot/dts/r7s72100-genmai.dts | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts > b/arch/arm/boot/dts/r7s72100-genmai.dts index ea649c7..0068a1a 100644 > --- a/arch/arm/boot/dts/r7s72100-genmai.dts > +++ b/arch/arm/boot/dts/r7s72100-genmai.dts > @@ -40,13 +40,19 @@ > > &pinctrl { > pinctrl-names = "default"; > - pinctrl-0 = <&scif2_pins>; > + pinctrl-0 = <&scif2_pins &i2c2_pins>; Referencing the SCIF2 and I2C2 pins nodes from the pinctrl node will result in the corresponding pins being configured when the pinctrl driver is probed. You could move this to the SCIF2 and I2C2 DT nodes, to have the pins configured as part of the probe sequence of the corresponding drivers. The advantage would be that pins would only be configured if the corresponding devices are enabled and get probed by a driver. This could save a bit of power if the boot loader configured the pins in low-power mode and the devices were unused (although in this very specific case I don't think it will make a difference). > > scif2_pins: serial2 { > /* P3_0 as TxD2; P3_2 as RxD2 */ > renesas-rz,pins = <RZ_PIN(3, 0) ALTERNATE_FUNC_6>, > <RZ_PIN(3, 2) ALTERNATE_FUNC_4>; > }; > + > + i2c2_pins: i2c2 { > + /* RIIC2: P1_4 as SCL, P1_5 as SDA */ > + renesas-rz,pins = <RZ_PIN(1, 4) ALTERNATE_FUNC_1>, > + <RZ_PIN(1, 5) ALTERNATE_FUNC_1>; > + }; > }; > > &extal_clk { -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* [RFC fixes 2/2] pinctrl: rz-pfc: Fix RZ/A1 pin function configuration 2017-01-27 16:47 ` [RFC fixes 0/2] FIX: " Jacopo Mondi 2017-01-27 16:47 ` [RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins Jacopo Mondi @ 2017-01-27 16:47 ` Jacopo Mondi 2017-01-27 21:09 ` [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver Chris Brandt 2 siblings, 0 replies; 54+ messages in thread From: Jacopo Mondi @ 2017-01-27 16:47 UTC (permalink / raw) To: laurent.pinchart, geert+renesas, linus.walleij, Chris.Brandt, wsa Cc: linux-renesas-soc, linux-gpio Fix alternate function configuration sequence for RZ/A1 SoC. The pin is first configured as simple input port, then alternate function is configured. Always enable input buffer for now as long as we don't have configuration paramters coming from device tree. Tested accessing embedded EEPROM chip through RIIC2 interface. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/pinctrl/rz-pfc/pinctrl-rza1.c | 55 +++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c index 221f048..53b71c8 100644 --- a/drivers/pinctrl/rz-pfc/pinctrl-rza1.c +++ b/drivers/pinctrl/rz-pfc/pinctrl-rza1.c @@ -27,6 +27,7 @@ #define MEM_RES_HIGH 1 /* PORTn_base + 0x4000 */ /* displacements from PORTn_base */ +#define PM_REG 0x300 #define PMC_REG 0x400 #define PFC_REG 0x500 #define PFCE_REG 0x600 @@ -235,8 +236,8 @@ static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg, #ifdef RZA1_REG_DBG u16 temp = ioread16(mem); - pr_err("%p %p %p - %4x %4x\n", - (void *)res->start, res->base, mem, temp, val); + pr_err("%p %p %p - %d:%d - %4x %4x\n", + (void *)res->start, res->base, mem, bank, pin, temp, val); #endif iowrite16(val, mem); } @@ -253,25 +254,35 @@ static inline void rza1_set_bit(struct rz_pinctrl_res *res, int reg, static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl, struct rz_pin_desc *pin_desc, unsigned int mux_mode) { - struct rz_pinctrl_res *res; + struct rz_pinctrl_res *res_low, *res_high; unsigned int bank = pin_desc->bank, pin = pin_desc->pin; + res_high = &pinctrl->res[MEM_RES_HIGH]; + res_low = &pinctrl->res[MEM_RES_LOW]; + /* - * disable input buffer and bi-control direction before entering - * alternate mode and let alternate function drive the IO mode by - * setting PIPCn to 1 + * reset pin state disabling input buffer and bi-directional control + * and configure it as input port before configuring alternate + * function later */ - res = &pinctrl->res[MEM_RES_HIGH]; - rza1_set_bit(res, PIBC_REG, bank, pin, 0); - rza1_set_bit(res, PBDC_REG, bank, pin, 0); - rza1_set_bit(res, PIPC_REG, bank, pin, 1); + rza1_set_bit(res_high, PIBC_REG, bank, pin, 0); + rza1_set_bit(res_high, PBDC_REG, bank, pin, 0); - /* TODO: - * all alternate functions except a few (3) need PIPCn = 1; - * find a way to identify those 3 functions, do not set PIPCn to 1 - * and set PMn according to some flag passed as parameter from DTS + rza1_set_bit(res_low, PM_REG, bank, pin, 1); + rza1_set_bit(res_low, PMC_REG, bank, pin, 0); + rza1_set_bit(res_high, PIPC_REG, bank, pin, 0); + + /* + * pins that has to be used as input, even in alternate function mode, + * needs input buffer enabled. + * Set PBDCn to enable bi-control which enables input buffer + * consequentialy. + * + * TODO: Add a flag to dts pin configuration to specify when a pin + * requires input buffer enabled and set PBDCn conditionally. */ + rza1_set_bit(res_high, PBDC_REG, bank, pin, 1); /* * enable alternate function mode and select it. @@ -290,11 +301,17 @@ static int rza1_set_mux(struct rz_pinctrl_dev *pinctrl, * 1 1 1 1 8 7 * ---------------------------------------------------- */ - res = &pinctrl->res[MEM_RES_LOW]; - rza1_set_bit(res, PMC_REG, bank, pin, 1); - rza1_set_bit(res, PFC_REG, bank, pin, mux_mode & 0x1); - rza1_set_bit(res, PFCE_REG, bank, pin, mux_mode & 0x2); - rza1_set_bit(res, PFCEA_REG, bank, pin, mux_mode & 0x4); + rza1_set_bit(res_low, PFC_REG, bank, pin, mux_mode & 0x1); + rza1_set_bit(res_low, PFCE_REG, bank, pin, mux_mode & 0x2); + rza1_set_bit(res_low, PFCEA_REG, bank, pin, mux_mode & 0x4); + + /* TODO: + * all alternate functions except a few (4) need PIPCn = 1; + * find a way to identify those 3 functions, do not set PIPCn to 1 + * and set PMn according to some flag passed as parameter from DTS + */ + rza1_set_bit(res_high, PIPC_REG, bank, pin, 1); + rza1_set_bit(res_low, PMC_REG, bank, pin, 1); return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* RE: [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver 2017-01-27 16:47 ` [RFC fixes 0/2] FIX: " Jacopo Mondi 2017-01-27 16:47 ` [RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins Jacopo Mondi 2017-01-27 16:47 ` [RFC fixes 2/2] pinctrl: rz-pfc: Fix RZ/A1 pin function configuration Jacopo Mondi @ 2017-01-27 21:09 ` Chris Brandt 2017-01-30 15:47 ` jacopo mondi 2 siblings, 1 reply; 54+ messages in thread From: Chris Brandt @ 2017-01-27 21:09 UTC (permalink / raw) To: Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij, wsa Cc: linux-renesas-soc, linux-gpio Hi Jacopo, On Friday, January 27, 2017, Jacopo Mondi wrote: > Hello, > sorry if I'm sending 2 patches on top of an RFC series with comments > still pending, but these patches enabled me to properly test pin > configuration sequence in order to access the internal EEPROM through > RIIC2 interface on pins 1_4 and 1_5. > > The outcome is a bugfix to RZ/A1 pincontroller driver which [2/2] applies > on. > > When sending v2 of the whole series I'll probably squash these, but if > someone is testing the RFC series I wanted to make sure he does not waste > his time with a broken driver. > > Thanks > j > > Jacopo Mondi (2): > arm: dts: genmai: Configure RIIC2 pins > pinctrl: rz-pfc: Fix RZ/A1 pin function configuration > > arch/arm/boot/dts/r7s72100-genmai.dts | 8 ++++- drivers/pinctrl/rz- > pfc/pinctrl-rza1.c | 55 +++++++++++++++++++++++------------ > 2 files changed, 43 insertions(+), 20 deletions(-) Preliminary testing shows that I2C pin muxing works. Nice job! Testing: - RZ/A1H RSK board - u-boot modified to make sure pins are put back to GPIO-IN - RIIC ch3 is connected to a I2C port expander that has 3 LEDs attached - using a heartbeat kernel thread that blinks the LEDs Of course, more testing is needed to make sure there is no "smoke and mirrors" going on like there was with the MSTP clock driver ;) Note that the I2C pin need to be configured at "bi-directional" but there is no way to specify that from DT, so that has to be added as a parameter. I am very happy about how easy it is to set the pins up! It almost matches what I do in our BSP code today, so I didn't even have to look at the HW schematic. I'll give Ethernet and SDHI a try. For RSPI, I have to go manually wire up a SPI flash again. Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver 2017-01-27 21:09 ` [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver Chris Brandt @ 2017-01-30 15:47 ` jacopo mondi 2017-01-30 16:29 ` Chris Brandt 0 siblings, 1 reply; 54+ messages in thread From: jacopo mondi @ 2017-01-30 15:47 UTC (permalink / raw) To: Chris Brandt, Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij, wsa Cc: linux-renesas-soc, linux-gpio Hi Chris, thanks for testing the series On 27/01/2017 22:09, Chris Brandt wrote: > Hi Jacopo, > > On Friday, January 27, 2017, Jacopo Mondi wrote: >> Hello, >> sorry if I'm sending 2 patches on top of an RFC series with comments >> still pending, but these patches enabled me to properly test pin >> configuration sequence in order to access the internal EEPROM through >> RIIC2 interface on pins 1_4 and 1_5. >> >> The outcome is a bugfix to RZ/A1 pincontroller driver which [2/2] applies >> on. >> >> When sending v2 of the whole series I'll probably squash these, but if >> someone is testing the RFC series I wanted to make sure he does not waste >> his time with a broken driver. >> >> Thanks >> j >> >> Jacopo Mondi (2): >> arm: dts: genmai: Configure RIIC2 pins >> pinctrl: rz-pfc: Fix RZ/A1 pin function configuration >> >> arch/arm/boot/dts/r7s72100-genmai.dts | 8 ++++- drivers/pinctrl/rz- >> pfc/pinctrl-rza1.c | 55 +++++++++++++++++++++++------------ >> 2 files changed, 43 insertions(+), 20 deletions(-) > > > Preliminary testing shows that I2C pin muxing works. Nice job! > > Testing: > - RZ/A1H RSK board > - u-boot modified to make sure pins are put back to GPIO-IN > - RIIC ch3 is connected to a I2C port expander that has 3 LEDs attached > - using a heartbeat kernel thread that blinks the LEDs > > Of course, more testing is needed to make sure there is no "smoke and mirrors" > going on like there was with the MSTP clock driver ;) > > > Note that the I2C pin need to be configured at "bi-directional" but there is > no way to specify that from DT, so that has to be added as a parameter. That's something I would like to discuss quite soon. One general thing I would like is having the so-called "additional parameters" part of the SoC driver module, as my gut feeling is that different PFC hw requires different configuration options. In example, the RZ/A1 SoC requires the input/output direction of the pin to be specified even when the alternate function is supposed to drive it (in order to enable/disable input buffer). The same applies to bi-directional port control as you pointed out in your example (bi-directional enables input buffer, but that's a consequence of how hw works there). I'm almost sure the same won't be required by all existing and forth-coming Renesas SoCs with a pin-based PFC hardware, but maybe other parameters I cannot think of right now will have to be specified instead. If we want to keep the configuration flags SoC-specific, the most easy way to pass them down from core module to SoC module would be compressing the alternate function number and other configurations in a single u32. In that case the dts will look like: renesasa,rz-pins = <BANK PORT (ALTERNATE_FUNC_# | IO_MODE | DIRECTION)>; We could even add a parameter to separate ALTERNATE_FUNCTION from the additional configurations, but I don't see any advantage in doing this at the moment. Can people with a broader knowledge of Renesas' SoC series ack/nack my assumptions here? Thanks j ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver 2017-01-30 15:47 ` jacopo mondi @ 2017-01-30 16:29 ` Chris Brandt 0 siblings, 0 replies; 54+ messages in thread From: Chris Brandt @ 2017-01-30 16:29 UTC (permalink / raw) To: jacopo mondi, Jacopo Mondi, laurent.pinchart, geert+renesas, linus.walleij, wsa Cc: linux-renesas-soc, linux-gpio Hi Jacopo, On Monday, January 30, 2017, Jacopo Mondi wrote: > > Note that the I2C pin need to be configured at "bi-directional" but > > there is no way to specify that from DT, so that has to be added as a > parameter. > > That's something I would like to discuss quite soon. > One general thing I would like is having the so-called "additional > parameters" part of the SoC driver module, as my gut feeling is that > different PFC hw requires different configuration options. In the API I used for the current RZ/A1 BSP, I used an extra parameter to pass in any non-normal settings. > In example, the RZ/A1 SoC requires the input/output direction of the pin > to be specified even when the alternate function is supposed to drive it > (in order to enable/disable input buffer). The same applies to bi- > directional port control as you pointed out in your example (bi- > directional enables input buffer, but that's a consequence of how hw works > there). > > I'm almost sure the same won't be required by all existing and forth- > coming Renesas SoCs with a pin-based PFC hardware, but maybe other > parameters I cannot think of right now will have to be specified instead. I would guess for one reason or another, there is always some exception in the hardware manual when it comes to pins settings. Meaning, there are restrictions in the I/O pad circuits that the HW designers find it easier to tell the SW guys to deal with it instead of trying to make it 'automatic' in HW. So...you'll always have the need for an extra parameter. > If we want to keep the configuration flags SoC-specific, the most easy way > to pass them down from core module to SoC module would be compressing the > alternate function number and other configurations in a single u32. > > In that case the dts will look like: > > renesasa,rz-pins = <BANK PORT (ALTERNATE_FUNC_# | IO_MODE | DIRECTION)>; > > We could even add a parameter to separate ALTERNATE_FUNCTION from the > additional configurations, but I don't see any advantage in doing this at > the moment. As I said, I made "additional configs" a separate parameter, but combining it with ALTERNATE_FUNC_# is fine with me. I have not seen an SoC that allowed any more than 8 different functions per pin, so you have lots of bits in u32 in order to set flags bits. However, as already mentioned by Geert, "ALTERNATE_FUNC_#" is too long of a name. Just "ALT_#" is fine. renesasa,rz-pins = <BANK PORT (ALT_# | IO_MODE | DIRECTION)>; Geert's suggestion was to just use the actual number (1-8), ALT_# might make it easier to decode if you are combining multiple things into a single u32. There is one more idea....but I heist to suggest it. You just pass in the port and alternate function number, and you do a lookup table for settings such as bi-direction or port direction register. You only put into the table the pin settings that are "not normal". The benefit is that the user doesn't have to know what pins needs what special tweak/flag/setting....but of course it start to make the driver more complex. Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 0/5] Renesas RZ series pinctrl driver 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi ` (7 preceding siblings ...) 2017-01-27 16:47 ` [RFC fixes 0/2] FIX: " Jacopo Mondi @ 2017-01-30 13:51 ` Linus Walleij 2017-01-30 15:53 ` Tony Lindgren 2017-01-30 16:08 ` Geert Uytterhoeven 8 siblings, 2 replies; 54+ messages in thread From: Linus Walleij @ 2017-01-30 13:51 UTC (permalink / raw) To: Jacopo Mondi, Laurent Pinchart, ext Tony Lindgren Cc: Geert Uytterhoeven, Linux-Renesas, linux-gpio On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > after having discussed in great detail the RZ series per-pin PFC hardware > peculiarities, this is a proposal for a possible pin-based pin controller > driver for SoC devices of Renesas RZ family. > > This RFC series adds a minimal driver infrastructure which supports pin > multiplexing via explicit per-pin settings performed in device tree sources. I think this is what Laurent had in mind when he said something about that he should have taken the per-pin approach from day 1. I would especially like to see Laurent's review and ACK on this series for that reason so we don't create something less than what he is expecting for the future of Renesas pin control. The series as such seems fine and is reusing Tony's work to generalize pin controllers putting functions and groups into the device tree in a nice way, which makes it relevant for him to have a look as well, if he has time. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 0/5] Renesas RZ series pinctrl driver 2017-01-30 13:51 ` [RFC 0/5] " Linus Walleij @ 2017-01-30 15:53 ` Tony Lindgren 2017-01-30 16:18 ` jacopo mondi 2017-01-30 16:08 ` Geert Uytterhoeven 1 sibling, 1 reply; 54+ messages in thread From: Tony Lindgren @ 2017-01-30 15:53 UTC (permalink / raw) To: Linus Walleij Cc: Jacopo Mondi, Laurent Pinchart, Geert Uytterhoeven, Linux-Renesas, linux-gpio * Linus Walleij <linus.walleij@linaro.org> [170130 05:53]: > On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > > > after having discussed in great detail the RZ series per-pin PFC hardware > > peculiarities, this is a proposal for a possible pin-based pin controller > > driver for SoC devices of Renesas RZ family. > > > > This RFC series adds a minimal driver infrastructure which supports pin > > multiplexing via explicit per-pin settings performed in device tree sources. > > I think this is what Laurent had in mind when he said something about > that he should have taken the per-pin approach from day 1. > > I would especially like to see Laurent's review and ACK on this series > for that reason so we don't create something less than what he is > expecting for the future of Renesas pin control. > > The series as such seems fine and is reusing Tony's work to generalize > pin controllers putting functions and groups into the device tree in a nice > way, which makes it relevant for him to have a look as well, if he has > time. Well I don't have the series in my inbox, but browsing through the patchwork seems OK. The things to check would be: 1. Do you guys really need RZ_PIN_DESC? Just let pinctrl framework generate the names, then do some simple user space tool that decodes the debugfs output. If you really need names, allow the consumer to set the name like we do for GPIOs and interrupts when requesting the resource. 2. Make sure you're using hardware offsets for any indexing to avoid adding artificial mapping tables between the driver and the the dts file. Many times new features are enabled between SoC revisions and a numbered index gets out of sync and needs constant patching just like we have for interrupt numbers earlier.. I don't think that's an issue with this series, but please check one more time for easy maintenance :) Regards, Tony ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 0/5] Renesas RZ series pinctrl driver 2017-01-30 15:53 ` Tony Lindgren @ 2017-01-30 16:18 ` jacopo mondi 0 siblings, 0 replies; 54+ messages in thread From: jacopo mondi @ 2017-01-30 16:18 UTC (permalink / raw) To: Tony Lindgren, Linus Walleij Cc: Jacopo Mondi, Laurent Pinchart, Geert Uytterhoeven, Linux-Renesas, linux-gpio Hi Tony, On 30/01/2017 16:53, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [170130 05:53]: >> On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: >> >>> after having discussed in great detail the RZ series per-pin PFC hardware >>> peculiarities, this is a proposal for a possible pin-based pin controller >>> driver for SoC devices of Renesas RZ family. >>> >>> This RFC series adds a minimal driver infrastructure which supports pin >>> multiplexing via explicit per-pin settings performed in device tree sources. >> >> I think this is what Laurent had in mind when he said something about >> that he should have taken the per-pin approach from day 1. >> >> I would especially like to see Laurent's review and ACK on this series >> for that reason so we don't create something less than what he is >> expecting for the future of Renesas pin control. >> >> The series as such seems fine and is reusing Tony's work to generalize >> pin controllers putting functions and groups into the device tree in a nice >> way, which makes it relevant for him to have a look as well, if he has >> time. > > Well I don't have the series in my inbox, but browsing through > the patchwork seems OK. The things to check would be: Sorry about this, I thought sending to linux-gpio was enough. We'll keep you in the loop now... > > 1. Do you guys really need RZ_PIN_DESC? Just let pinctrl > framework generate the names, then do some simple user > space tool that decodes the debugfs output. If you really > need names, allow the consumer to set the name like we do > for GPIOs and interrupts when requesting the resource. > Well, more than names I have used the RZ_PIN_DESC macro as an extended version of the PINCTRL_PIN one, to set bank and pin fields of the rz_pin_desc structure that describes a pin. The proposed DTS syntax identifies pins by their position (bank:pin) instead that by index, so I need to have those information available in pin description to identify them. > 2. Make sure you're using hardware offsets for any indexing > to avoid adding artificial mapping tables between the driver > and the the dts file. Many times new features are enabled > between SoC revisions and a numbered index gets out of sync > and needs constant patching just like we have for interrupt > numbers earlier.. I don't think that's an issue with this > series, but please check one more time for easy maintenance :) > As we describe pins "by position", if I got your comment properly, we should be safe, as indexes are purely incremental values from [0-npins]. I know pinctrl-single (and other drivers such as imx one if I'm not wrong) uses pin indexes to calculate register offsets. This series doesn't but uses the [bank:pin] couple for that purpose instead. Thanks j > Regards, > > Tony > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 0/5] Renesas RZ series pinctrl driver 2017-01-30 13:51 ` [RFC 0/5] " Linus Walleij 2017-01-30 15:53 ` Tony Lindgren @ 2017-01-30 16:08 ` Geert Uytterhoeven 2017-01-30 18:59 ` Laurent Pinchart 1 sibling, 1 reply; 54+ messages in thread From: Geert Uytterhoeven @ 2017-01-30 16:08 UTC (permalink / raw) To: Linus Walleij Cc: Jacopo Mondi, Laurent Pinchart, ext Tony Lindgren, Geert Uytterhoeven, Linux-Renesas, linux-gpio Hi Linus, On Mon, Jan 30, 2017 at 2:51 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: >> after having discussed in great detail the RZ series per-pin PFC hardware >> peculiarities, this is a proposal for a possible pin-based pin controller >> driver for SoC devices of Renesas RZ family. >> >> This RFC series adds a minimal driver infrastructure which supports pin >> multiplexing via explicit per-pin settings performed in device tree sources. > > I think this is what Laurent had in mind when he said something about > that he should have taken the per-pin approach from day 1. > > I would especially like to see Laurent's review and ACK on this series > for that reason so we don't create something less than what he is > expecting for the future of Renesas pin control. It depends on the actual hardware: while per-pin settings are suitable for SoCs that have per-pin hardware configuration (e.g. RZ/A1), it's not suitable for SoCs where that's not the case, and where the hardware has group-wise configuration (e.g. on R-Car). Hence IMHO "the future of Renesas pin control" will remain a split personality for a while ;-) Laurent: please kick me if you disagree... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [RFC 0/5] Renesas RZ series pinctrl driver 2017-01-30 16:08 ` Geert Uytterhoeven @ 2017-01-30 18:59 ` Laurent Pinchart 2017-01-30 19:49 ` Chris Brandt 0 siblings, 1 reply; 54+ messages in thread From: Laurent Pinchart @ 2017-01-30 18:59 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Linus Walleij, Jacopo Mondi, ext Tony Lindgren, Geert Uytterhoeven, Linux-Renesas, linux-gpio Hi Geert, On Monday 30 Jan 2017 17:08:11 Geert Uytterhoeven wrote: > On Mon, Jan 30, 2017 at 2:51 PM, Linus Walleij wrote: > > On Wed, Jan 25, 2017 at 7:09 PM, Jacopo Mondi wrote: > >> after having discussed in great detail the RZ series per-pin PFC > >> hardware peculiarities, this is a proposal for a possible pin-based pin > >> controller driver for SoC devices of Renesas RZ family. > >> > >> This RFC series adds a minimal driver infrastructure which supports pin > >> multiplexing via explicit per-pin settings performed in device tree > >> sources. > > > > I think this is what Laurent had in mind when he said something about > > that he should have taken the per-pin approach from day 1. > > > > I would especially like to see Laurent's review and ACK on this series > > for that reason so we don't create something less than what he is > > expecting for the future of Renesas pin control. > > It depends on the actual hardware: while per-pin settings are suitable for > SoCs that have per-pin hardware configuration (e.g. RZ/A1), it's not > suitable for SoCs where that's not the case, and where the hardware has > group-wise configuration (e.g. on R-Car). > > Hence IMHO "the future of Renesas pin control" will remain a split > personality for a while ;-) > > Laurent: please kick me if you disagree... I unfortunately agree. Maybe we could try to speed up the process by providing feedback to the hardware engineers ? -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: [RFC 0/5] Renesas RZ series pinctrl driver 2017-01-30 18:59 ` Laurent Pinchart @ 2017-01-30 19:49 ` Chris Brandt 0 siblings, 0 replies; 54+ messages in thread From: Chris Brandt @ 2017-01-30 19:49 UTC (permalink / raw) To: Laurent Pinchart, Geert Uytterhoeven Cc: Linus Walleij, Jacopo Mondi, ext Tony Lindgren, Geert Uytterhoeven, Linux-Renesas, linux-gpio On Monday, January 30, 2017, Laurent Pinchart wrote: > > It depends on the actual hardware: while per-pin settings are suitable > > for SoCs that have per-pin hardware configuration (e.g. RZ/A1), it's > > not suitable for SoCs where that's not the case, and where the > > hardware has group-wise configuration (e.g. on R-Car). > > > > Hence IMHO "the future of Renesas pin control" will remain a split > > personality for a while ;-) > > > > Laurent: please kick me if you disagree... > > I unfortunately agree. Maybe we could try to speed up the process by > providing feedback to the hardware engineers ? For what it's worth, I fired off my 'rant and rave' last week about the current R-Car PFC block so I'll be curious to see what they say. Of course, I'm on the RZ side of the company, so maybe someone on the R-Car side might have better luck. Chris ^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2017-02-07 10:27 UTC | newest] Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-25 18:09 [RFC 0/5] Renesas RZ series pinctrl driver Jacopo Mondi 2017-01-25 18:09 ` [RFC 1/5] pinctrl: rz-pfc: Add Renesas RZ pinctrl core module Jacopo Mondi 2017-01-26 2:58 ` Chris Brandt 2017-01-26 9:08 ` jacopo mondi 2017-01-26 14:19 ` Chris Brandt 2017-01-26 19:43 ` Geert Uytterhoeven 2017-01-30 19:19 ` Chris Brandt 2017-01-31 9:00 ` jacopo mondi 2017-01-31 13:48 ` Chris Brandt 2017-02-01 12:47 ` Laurent Pinchart 2017-02-01 15:21 ` Laurent Pinchart 2017-02-06 18:15 ` jacopo mondi 2017-02-06 18:28 ` Tony Lindgren 2017-02-07 10:27 ` jacopo mondi 2017-01-25 18:09 ` [RFC 2/5] pinctrl: rz-pfc: Add Renesas RZ/A1 pinctrl driver Jacopo Mondi 2017-01-26 19:49 ` Geert Uytterhoeven 2017-01-30 19:19 ` Chris Brandt 2017-01-31 10:39 ` Laurent Pinchart 2017-01-31 14:11 ` Chris Brandt 2017-02-01 15:25 ` Laurent Pinchart 2017-01-25 18:09 ` [RFC 3/5] arm: dts: dt-bindings: Add Renesas RZ pinctrl header Jacopo Mondi 2017-01-26 19:52 ` Geert Uytterhoeven 2017-01-30 18:30 ` Laurent Pinchart 2017-01-31 9:12 ` jacopo mondi 2017-01-31 9:31 ` Geert Uytterhoeven 2017-01-31 14:00 ` Chris Brandt 2017-01-25 18:09 ` [RFC 4/5] arm: dts: r7s1000: Add pincontroller node Jacopo Mondi 2017-01-26 9:49 ` Sergei Shtylyov 2017-01-26 19:54 ` Geert Uytterhoeven 2017-01-31 10:24 ` jacopo mondi 2017-01-31 10:34 ` Geert Uytterhoeven 2017-01-30 18:29 ` Laurent Pinchart 2017-01-30 19:39 ` Chris Brandt 2017-01-31 10:35 ` Laurent Pinchart 2017-01-31 14:08 ` Chris Brandt 2017-01-25 18:09 ` [RFC 5/5] arm: dts: genmai: Add SCIF2 pin group Jacopo Mondi 2017-01-26 9:51 ` Sergei Shtylyov 2017-01-30 18:17 ` Laurent Pinchart 2017-01-30 18:55 ` Geert Uytterhoeven 2017-01-26 2:57 ` [RFC 0/5] Renesas RZ series pinctrl driver Chris Brandt 2017-01-26 19:56 ` Geert Uytterhoeven 2017-01-27 16:47 ` [RFC fixes 0/2] FIX: " Jacopo Mondi 2017-01-27 16:47 ` [RFC fixes 1/2] arm: dts: genmai: Configure RIIC2 pins Jacopo Mondi 2017-01-30 18:49 ` Laurent Pinchart 2017-01-27 16:47 ` [RFC fixes 2/2] pinctrl: rz-pfc: Fix RZ/A1 pin function configuration Jacopo Mondi 2017-01-27 21:09 ` [RFC fixes 0/2] FIX: Renesas RZ series pinctrl driver Chris Brandt 2017-01-30 15:47 ` jacopo mondi 2017-01-30 16:29 ` Chris Brandt 2017-01-30 13:51 ` [RFC 0/5] " Linus Walleij 2017-01-30 15:53 ` Tony Lindgren 2017-01-30 16:18 ` jacopo mondi 2017-01-30 16:08 ` Geert Uytterhoeven 2017-01-30 18:59 ` Laurent Pinchart 2017-01-30 19:49 ` Chris Brandt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.