Hi, On 20 November 2016 at 08:38, <kos...@marvell.com> wrote: > From: Konstantin Porotchkin <kos...@marvell.com> > > Add a port of Marvell pin control driver. > The A8K SoC family contains several silicone dies interconnected > in a single package. Every die is normally equuipped with its own > pin controller unit. > Since the UCLASS_PINCTRL device only calls the probe method for > the first detected pin controller, a trick similar to used with > comphy driver is required. > In order to bring up all pin controllers available in A8K SoC, > the arch_early_init_r() function sequentially calls the > uclass_get_device() function for each UCLASS_PINCTRL device. > > Change-Id: Iff143827e8f1558a554d77173562c4b52ce179d7 > Signed-off-by: Konstantin Porotchkin <kos...@marvell.com> > Cc: Simon Glass <s...@chromium.org> > Cc: Stefan Roese <s...@denx.de> > Cc: Nadav Haklai <nad...@marvell.com> > Cc: Neta Zur Hershkovits <n...@marvell.com> > Cc: Omri Itach <om...@marvell.com> > Cc: Igal Liberman <ig...@marvell.com> > Cc: Haim Boot <ha...@marvell.com> > Cc: Hanna Hawa <han...@marvell.com> > --- > arch/arm/include/asm/arch-armada8k/soc-info.h | 45 +++++ > arch/arm/mach-mvebu/arm64-common.c | 1 - > .../pinctrl/marvell,mvebu-pinctrl.txt | 113 ++++++++++++ > drivers/pinctrl/Kconfig | 1 + > drivers/pinctrl/Makefile | 1 + > drivers/pinctrl/mvebu/Kconfig | 7 + > drivers/pinctrl/mvebu/Makefile | 17 ++ > drivers/pinctrl/mvebu/pinctrl-mvebu.c | 195 > +++++++++++++++++++++ > drivers/pinctrl/mvebu/pinctrl-mvebu.h | 44 +++++ > 9 files changed, 423 insertions(+), 1 deletion(-) > create mode 100644 arch/arm/include/asm/arch-armada8k/soc-info.h > create mode 100644 doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt > create mode 100644 drivers/pinctrl/mvebu/Kconfig > create mode 100644 drivers/pinctrl/mvebu/Makefile > create mode 100644 drivers/pinctrl/mvebu/pinctrl-mvebu.c > create mode 100644 drivers/pinctrl/mvebu/pinctrl-mvebu.h >
Generally looks good but I have a load of nits sorry. > diff --git a/arch/arm/include/asm/arch-armada8k/soc-info.h > b/arch/arm/include/asm/arch-armada8k/soc-info.h > new file mode 100644 > index 0000000..4640deb > --- /dev/null > +++ b/arch/arm/include/asm/arch-armada8k/soc-info.h > @@ -0,0 +1,45 @@ > +/* > + * > *************************************************************************** > + * Copyright (C) 2015 Marvell International Ltd. > + * > *************************************************************************** > + * This program is free software: you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation, either version 2 of the License, or any later > version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + * > *************************************************************************** Can you please use SPDX? > + */ > + > +#ifndef _SOC_INFO_H_ > +#define _SOC_INFO_H_ > + > +/* General MPP definitions */ > +#define MAX_MPP_OPTS 7 > +#define MAX_MPP_ID 15 > + > +#define MPP_BIT_CNT 4 > +#define MPP_FIELD_MASK 0x7 > +#define MPP_FIELD_BITS 3 > +#define MPP_VAL_MASK 0xF > + > +#define MPPS_PER_REG (32 / MPP_BIT_CNT) > +#define MAX_MPP_REGS ((MAX_MPP_ID + MPPS_PER_REG) / MPPS_PER_REG) > + > +/* MPP pins and functions correcsponding to UART RX connections > + This information is used for detection of recovery boot mode (boot from > UART) */ /* * MPP pins * ... * / > +#define MPP_UART_RX_PINS { 3, 5 } > +#define MPP_UART_RX_FUNCTIONS { 1, 2 } > + > +/* Pin Ctrl driver definitions */ > +#define BITS_PER_PIN 4 > +#define PIN_FUNC_MASK ((1 << BITS_PER_PIN) - 1) > +#define PIN_REG_SHIFT 3 > +#define PIN_FIELD_MASK ((1 << PIN_REG_SHIFT) - 1) > + > +#endif /* _SOC_INFO_H_ */ > diff --git a/arch/arm/mach-mvebu/arm64-common.c > b/arch/arm/mach-mvebu/arm64-common.c > index 1fc2ff2..78fe7a7 100644 > --- a/arch/arm/mach-mvebu/arm64-common.c > +++ b/arch/arm/mach-mvebu/arm64-common.c > @@ -124,7 +124,6 @@ int arch_early_init_r(void) > if (ret) > break; > } > - Unrelated change? > /* Cause the SATA device to do its early init */ > uclass_first_device(UCLASS_AHCI, &dev); > > diff --git a/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt > b/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt > new file mode 100644 > index 0000000..0973db8 > --- /dev/null > +++ b/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt > @@ -0,0 +1,113 @@ > +The pinctrl driver enables Marvell Armada 8K SoCs to configure the > multi-purpose > +pins (mpp) to a specific function. > +A Marvell SoC pin configuration node is a node of a group of pins which can > +be used for a specific device or function. Each node requires one or more > +mpp pins or group of pins and a mpp function common to all pins. > + > +Required properties for the pinctrl driver: > +- compatible: "marvell,mvebu-pinctrl", > + "marvell,armada-ap806-pinctrl", > + "marvell,a70x0-pinctrl", > + "marvell,a80x0-cp0-pinctrl", > + "marvell,a80x0-cp1-pinctrl" > +- bank-name: A string defining the pinc controller bank name > +- reg: A pair of values defining the pin controller base > address > + and the address space > +- pin-count: Numeric value defining the amount of multi purpose pins > + included in this bank > +- max-func: Numeric value defining the maximum function value for > + pins in this bank > +- pin-func: Array of pin function values for every pin in the bank. > + When the function value for a specific pin equal 0xFF, > + the pin configuration is skipped and a default function > + value is used for this pin. > + > +The A8K is a hybrid SoC that contains several silicon dies interconnected in > +a single package. Each such die may have a separate pin controller. > + > +Example: > +/ { > + ap806 { > + config-space { > + pinctl: pinctl@6F4000 { > + compatible = "marvell,mvebu-pinctrl", > + "marvell,armada-ap806-pinctrl"; > + bank-name ="apn-806"; > + reg = <0x6F4000 0x10>; > + pin-count = <20>; > + max-func = <3>; > + /* MPP Bus: > + SPI0 [0-3] > + I2C0 [4-5] > + UART0 [11,19] > + */ > + /* 0 1 2 3 4 5 6 7 8 9 */ > + pin-func = < 3 3 3 3 3 3 0 0 0 0 > + 0 3 0 0 0 0 0 0 0 3>; > + }; > + }; > + }; > + > + cp110-master { > + config-space { > + cpm_pinctl: pinctl@44000 { > + compatible = "marvell,mvebu-pinctrl", > + "marvell,a70x0-pinctrl", > + "marvell,a80x0-cp0-pinctrl"; > + bank-name ="cp0-110"; > + reg = <0x440000 0x20>; > + pin-count = <63>; > + max-func = <0xf>; > + /* MPP Bus: > + [0-31] = 0xff: Keep default > CP0_shared_pins: > + [11] CLKOUT_MPP_11 (out) > + [23] LINK_RD_IN_CP2CP (in) > + [25] CLKOUT_MPP_25 (out) > + [29] AVS_FB_IN_CP2CP (in) > + [32,34] SMI > + [31] GPIO: push button/Wake > + [35-36] GPIO > + [37-38] I2C > + [40-41] SATA[0/1]_PRESENT_ACTIVEn > + [42-43] XSMI > + [44-55] RGMII1 > + [56-62] SD > + */ > + /* 0 1 2 3 4 5 6 > 7 8 9 */ > + pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff > 0xff 0xff 0xff 0xff > + 0xff 0xff 0xff 0xff 0xff 0xff > 0xff 0xff 0xff 0xff > + 0xff 0xff 0xff 0xff 0xff 0xff > 0xff 0xff 0xff 0xff > + 0xff 0 7 0 7 0 0 > 2 2 0 > + 0 0 8 8 1 1 1 > 1 1 1 > + 1 1 1 1 1 1 > 0xE 0xE 0xE 0xE > + 0xE 0xE 0xE>; > + }; > + }; > + }; > + > + cp110-slave { > + config-space { > + cps_pinctl: pinctl@44000 { > + compatible = "marvell,mvebu-pinctrl", > + "marvell,a80x0-cp1-pinctrl"; > + bank-name ="cp1-110"; > + reg = <0x440000 0x20>; > + pin-count = <63>; > + max-func = <0xf>; > + /* MPP Bus: > + [0-11] RGMII0 > + [27,31] GE_MDIO/MDC > + [32-62] = 0xff: Keep default > CP1_shared_pins: > + */ > + /* 0 1 2 3 4 5 6 > 7 8 9 */ > + pin-func = < 0x3 0x3 0x3 0x3 0x3 0x3 > 0x3 0x3 0x3 0x3 > + 0x3 0x3 0xff 0xff 0xff 0xff > 0xff 0xff 0xff 0xff > + 0xff 0xff 0xff 0xff 0xff 0xff > 0xff 0x8 0xff 0xff > + 0xff 0x8 0xff 0xff 0xff 0xff > 0xff 0xff 0xff 0xff > + 0xff 0xff 0xff 0xff 0xff 0xff > 0xff 0xff 0xff 0xff > + 0xff 0xff 0xff 0xff 0xff 0xff > 0xff 0xff 0xff 0xff > + 0xff 0xff 0xff>; > + }; > + }; > + }; > +} > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig > index 12be3cf..efcb4c0 100644 > --- a/drivers/pinctrl/Kconfig > +++ b/drivers/pinctrl/Kconfig > @@ -181,5 +181,6 @@ source "drivers/pinctrl/meson/Kconfig" > source "drivers/pinctrl/nxp/Kconfig" > source "drivers/pinctrl/uniphier/Kconfig" > source "drivers/pinctrl/exynos/Kconfig" > +source "drivers/pinctrl/mvebu/Kconfig" > > endmenu > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile > index f28b5c1..512112a 100644 > --- a/drivers/pinctrl/Makefile > +++ b/drivers/pinctrl/Makefile > @@ -15,3 +15,4 @@ obj-$(CONFIG_PINCTRL_UNIPHIER) += uniphier/ > obj-$(CONFIG_PIC32_PINCTRL) += pinctrl_pic32.o > obj-$(CONFIG_PINCTRL_EXYNOS) += exynos/ > obj-$(CONFIG_PINCTRL_MESON) += meson/ > +obj-$(CONFIG_PINCTRL_MVEBU) += mvebu/ > diff --git a/drivers/pinctrl/mvebu/Kconfig b/drivers/pinctrl/mvebu/Kconfig > new file mode 100644 > index 0000000..cf9c299 > --- /dev/null > +++ b/drivers/pinctrl/mvebu/Kconfig > @@ -0,0 +1,7 @@ > +config PINCTRL_MVEBU > + depends on ARCH_MVEBU > + bool > + default y > + help > + Support pin multiplexing and pin configuration control on > + Marvell's Armada-8K SoC. > diff --git a/drivers/pinctrl/mvebu/Makefile b/drivers/pinctrl/mvebu/Makefile > new file mode 100644 > index 0000000..7db2b97 > --- /dev/null > +++ b/drivers/pinctrl/mvebu/Makefile > @@ -0,0 +1,17 @@ > +#* > *************************************************************************** > +#* Copyright (C) 2016 Marvell International Ltd. > +#* > *************************************************************************** > +#* This program is free software: you can redistribute it and/or modify it > +#* under the terms of the GNU General Public License as published by the Free > +#* Software Foundation, either version 2 of the License, or any later > version. > +#* > +#* This program is distributed in the hope that it will be useful, > +#* but WITHOUT ANY WARRANTY; without even the implied warranty of > +#* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +#* GNU General Public License for more details. > +#* > +#* You should have received a copy of the GNU General Public License > +#* along with this program. If not, see <http://www.gnu.org/licenses/>. > +#* > *************************************************************************** > + > +obj-$(CONFIG_PINCTRL_MVEBU) += pinctrl-mvebu.o > diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c > b/drivers/pinctrl/mvebu/pinctrl-mvebu.c > new file mode 100644 > index 0000000..02fcd2f > --- /dev/null > +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c > @@ -0,0 +1,195 @@ > +/* > + * > *************************************************************************** > + * Copyright (C) 2016 Marvell International Ltd. > + * > *************************************************************************** > + * This program is free software: you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation, either version 2 of the License, or any later > version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + * > *************************************************************************** > + */ > + > +#include <config.h> > +#include <common.h> > +#include <dm.h> > +#include <asm/system.h> > +#include <asm/io.h> > +#include <dm/pinctrl.h> > +#include <dm/root.h> > +#include <fdtdec.h> > +#include <errno.h> > +#include <asm/arch/fdt.h> > +#include <asm/arch-armada8k/soc-info.h> > +#include "pinctrl-mvebu.h" Please check include-file ordering here: http://www.denx.de/wiki/U-Boot/CodingStyle It is close. > + > +/* > + * mvebu_pinctrl_set_state: configure pin functions. > + * dev: the pinctrl device to be configured. > + * config: the state to be configured. /* * mvebu_pinctrl_set_state() - configure pin functions * @dev: the pinctrl device to be configured. * @config: the state to be configured. * @return ... */ Please use that consistently. > + */ > +int mvebu_pinctrl_set_state(struct udevice *dev, struct udevice *config) > +{ > + const void *blob = gd->fdt_blob; > + int node = config->of_offset; > + struct mvebu_pinctrl_priv *priv; > + u32 pin_arr[CONFIG_MAX_PINS_PER_BANK]; > + u32 function; > + int i, pin_count; > + > + priv = dev_get_priv(dev); > + if (!priv) { > + printf("%s: Failed to get private\n", __func__); debug() > + return -EINVAL; > + } > + > + pin_count = fdtdec_get_int_array_count(blob, node, "marvell,pins", > pin_arr, CONFIG_MAX_PINS_PER_BANK); Are you sure this passes checkpatch? That line seems to long. > + if (pin_count <= 0) { > + error("Failed reading pins array for pinconfig %s (%d)\n", > config->name, pin_count); debug() to avoid bloating the code? (globally) > + return -EINVAL; > + } > + > + function = fdtdec_get_int(blob, node, "marvell,function", 0xFF); nit: Lower case hex throughout > + > + for (i = 0; i < pin_count; i++) { > + int reg_offset; > + int field_offset; > + u32 reg, mask; > + int pin = pin_arr[i]; > + > + if (function > priv->max_func) { > + error("Illegal function %d for pinconfig %s\n", > function, config->name); > + return -EINVAL; > + } > + > + /* Calculate register address and bit in register */ > + reg_offset = priv->reg_direction * 4 * (pin >> > (PIN_REG_SHIFT)); > + field_offset = (BITS_PER_PIN) * (pin & PIN_FIELD_MASK); > + mask = ~(PIN_FUNC_MASK << field_offset); > + > + /* Clip value to field resolution */ > + function &= PIN_FUNC_MASK; > + > + reg = readl(priv->base_reg + reg_offset); > + reg = (reg & mask) | (function << field_offset); > + writel(reg, priv->base_reg + reg_offset); You can use clrsetbits_le32() > + } > + > + return 0; > +} > + > +/* > + * mvebu_pinctrl_set_state_all: configure the entire bank pin functions. > + * dev: the pinctrl device to be configured. > + * config: the state to be configured. > + */ > +static int mvebu_pinctrl_set_state_all(struct udevice *dev, struct udevice > *config) > +{ > + const void *blob = gd->fdt_blob; > + int node = config->of_offset; > + struct mvebu_pinctrl_priv *priv; > + u32 func_arr[CONFIG_MAX_PINS_PER_BANK]; > + int pin, err; > + > + priv = dev_get_priv(dev); > + if (!priv) { > + printf("%s: Failed to get private\n", __func__); This cannot happen if the device is probed. Add an assert() if you are paranoid, but it has not benefit. > + return -EINVAL; > + } > + > + err = fdtdec_get_int_array(blob, node, "pin-func", func_arr, > priv->pin_cnt); > + if (err) { > + error("Failed reading pin functions for bank %s\n", > priv->bank_name); > + return -EINVAL; > + } > + > + for (pin = 0; pin < priv->pin_cnt; pin++) { > + int reg_offset; > + int field_offset; > + u32 reg, mask; > + u32 func = func_arr[pin]; > + > + /* Bypass pins with function 0xFF */ > + if (func == 0xFF) { > + debug("Warning: pin %d value is not modified (kept as > default\n", pin); > + continue; > + } else if (func > priv->max_func) { > + error("Illegal function %d for pin %d\n", func, pin); > + return -EINVAL; > + } > + > + /* Calculate register address and bit in register */ > + reg_offset = priv->reg_direction * 4 * (pin >> > (PIN_REG_SHIFT)); > + field_offset = (BITS_PER_PIN) * (pin & PIN_FIELD_MASK); > + mask = ~(PIN_FUNC_MASK << field_offset); > + > + /* Clip value to field resolution */ > + func &= PIN_FUNC_MASK; > + > + reg = readl(priv->base_reg + reg_offset); > + reg = (reg & mask) | (func << field_offset); > + writel(reg, priv->base_reg + reg_offset); clrsetbits_le32() > + } > + > + return 0; > +} > + > +int mvebu_pinctl_probe(struct udevice *dev) > +{ > + const void *blob = gd->fdt_blob; > + int node = dev->of_offset; > + struct mvebu_pinctrl_priv *priv; > + fdt_addr_t base; > + > + priv = dev_get_priv(dev); > + if (!priv) { > + printf("%s: Failed to get private\n", __func__); debug() - please fix globally - drivers should not print messages. > + return -EINVAL; > + } > + > + base = dev_get_addr(dev); > + if (base == FDT_ADDR_T_NONE) { > + printf("%s: Failed to get base address\n", __func__); > + return -EINVAL; > + } > + > + priv->base_reg = (u8 *)base; Or use dev_get_addr_ptr() ? > + priv->pin_cnt = fdtdec_get_int(blob, node, "pin-count", > CONFIG_MAX_PINS_PER_BANK); > + priv->max_func = fdtdec_get_int(blob, node, "max-func", > CONFIG_MAX_FUNC); > + priv->bank_name = fdt_getprop(blob, node, "bank-name", NULL); > + > + priv->reg_direction = 1; > + if (fdtdec_get_bool(blob, node, "reverse-reg")) > + priv->reg_direction = -1; > + > + return mvebu_pinctrl_set_state_all(dev, dev); > +} > + > + > +static struct pinctrl_ops mvebu_pinctrl_ops = { > + .set_state = mvebu_pinctrl_set_state > +}; > + > +static const struct udevice_id mvebu_pinctrl_ids[] = { > + { .compatible = "marvell,mvebu-pinctrl" }, > + { .compatible = "marvell,armada-ap806-pinctrl" }, > + { .compatible = "marvell,a70x0-pinctrl" }, > + { .compatible = "marvell,a80x0-cp0-pinctrl" }, > + { .compatible = "marvell,a80x0-cp1-pinctrl" }, > + { } > +}; > + > +U_BOOT_DRIVER(pinctrl_mvebu) = { > + .name = "mvebu_pinctrl", > + .id = UCLASS_PINCTRL, > + .of_match = mvebu_pinctrl_ids, > + .priv_auto_alloc_size = sizeof(struct mvebu_pinctrl_priv), > + .ops = &mvebu_pinctrl_ops, > + .probe = mvebu_pinctl_probe > +}; > diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.h > b/drivers/pinctrl/mvebu/pinctrl-mvebu.h > new file mode 100644 > index 0000000..61c84ce > --- /dev/null > +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.h > @@ -0,0 +1,44 @@ > +/* > + * > *************************************************************************** > + * Copyright (C) 2016 Marvell International Ltd. > + * > *************************************************************************** > + * This program is free software: you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation, either version 2 of the License, or any later > version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + * > *************************************************************************** > + */ > + > + #ifndef __PINCTRL_MVEBU_H_ > + #define __PINCTRL_MVEBU_H_ > + > + #define CONFIG_MAX_PINCTL_BANKS 4 > + #define CONFIG_MAX_PINS_PER_BANK 100 > + #define CONFIG_MAX_FUNC 0xF Avoid using CONFIG_ prefixes since this looks like a new global CONFIG option. Just dropping the CONFIG_ will do. > + > +DECLARE_GLOBAL_DATA_PTR; Please put that in the C file that needs it. > + > +/* > + * struct mvebu_pin_bank_data: mvebu-pinctrl bank data * struct mvebu_pin_bank_data - mvebu-pinctrl bank data > + * @base_reg: controller base address for this bank > + * @pin_cnt: number of ping included in this bank ping? > + * @max_func: maximum configurable function value for pins in this bank > + * @reg_direction: ? > + * @bank_name: the pins bank name pin's > + */ > +struct mvebu_pinctrl_priv { > + u8 *base_reg; > + u32 pin_cnt; > + u32 max_func; Why are these u32? Can they be uint? > + int reg_direction; > + const char *bank_name; > +}; > + > +#endif /* __PINCTRL_MVEBU_H_ */ > -- > 2.7.4 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
