All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
@ 2020-05-26 12:19 Stefan Roese
  2020-05-31 14:08 ` Simon Glass
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Stefan Roese @ 2020-05-26 12:19 UTC (permalink / raw)
  To: u-boot

From: Suneel Garapati <sgarapati@marvell.com>

Add support for GPIO controllers found on Octeon II/III and Octeon TX
TX2 SoC platforms.

Signed-off-by: Aaron Williams <awilliams@marvell.com>
Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
Cc: Aaron Williams <awilliams@marvell.com>
Cc: Chandrakala Chavva <cchavva@marvell.com>
---
v2 (Stefan):
- Removed #ifdef's for Octeon vs OcteonTX/TX2 completely
  The differentiation is now made via driver data / compatible
  string

RFC -> v1 (Stefan)
- Separated this patch from the OcteonTX/TX2 RFC patch series into a
  single patch. This is useful, as the upcoming MIPS Octeon support will
  use this GPIO driver.
- Added MIPS Octeon II/III support (big endian). Rename driver and its
  function names from "octeontx" to "octeon" to better match all Octeon
  platforms.
- Moved from union to defines / bitmasks. This makes the driver usage
  on little- and big-endian platforms much easier.
- Used clrbits_64() instead of clrbits_le64() and friends to support
  usage on little- and big-endian systems
- Removed dev->req_seq assignment
- Enhanced Kconfig text
- Rewrote GPIO_BIT macro
- Dropped many macros to calculate the registers offsets and implemented
  simple functions for this (easier to read)
- Used GENMASK_ULL and FIELD_GET helpers
- Minor cosmetic changes (dropped brackets etc)
- Reword commit text and subject

 drivers/gpio/Kconfig       |  10 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/octeon_gpio.c | 253 +++++++++++++++++++++++++++++++++++++
 3 files changed, 264 insertions(+)
 create mode 100644 drivers/gpio/octeon_gpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d87f6cc105..451896f400 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -316,6 +316,16 @@ config PIC32_GPIO
 	help
 	  Say yes here to support Microchip PIC32 GPIOs.
 
+config OCTEON_GPIO
+	bool "Octeon II/III/TX/TX2 GPIO driver"
+	depends on DM_GPIO && (ARCH_OCTEON || ARCH_OCTEONTX || ARCH_OCTEONTX2)
+	default y
+	help
+	  Add support for the Marvell Octeon GPIO driver. This is used with
+	  various Octeon parts such as Octeon II/III and OcteonTX/TX2.
+	  Octeon II/III has 32 GPIOs (count defined via DT) and OcteonTX/TX2
+	  has 64 GPIOs (count defined via internal register).
+
 config STM32_GPIO
 	bool "ST STM32 GPIO driver"
 	depends on DM_GPIO && (ARCH_STM32 || ARCH_STM32MP)
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 7638259007..eb6364adb4 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_HIKEY_GPIO)	+= hi6220_gpio.o
 obj-$(CONFIG_HSDK_CREG_GPIO)	+= hsdk-creg-gpio.o
 obj-$(CONFIG_IMX_RGPIO2P)	+= imx_rgpio2p.o
 obj-$(CONFIG_PIC32_GPIO)	+= pic32_gpio.o
+obj-$(CONFIG_OCTEON_GPIO)	+= octeon_gpio.o
 obj-$(CONFIG_MVEBU_GPIO)	+= mvebu_gpio.o
 obj-$(CONFIG_MSM_GPIO)		+= msm_gpio.o
 obj-$(CONFIG_$(SPL_)PCF8575_GPIO)	+= pcf8575_gpio.o
diff --git a/drivers/gpio/octeon_gpio.c b/drivers/gpio/octeon_gpio.c
new file mode 100644
index 0000000000..d7ac9a1910
--- /dev/null
+++ b/drivers/gpio/octeon_gpio.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier:    GPL-2.0
+/*
+ * Copyright (C) 2018 Marvell International Ltd.
+ *
+ * (C) Copyright 2011
+ * eInfochips Ltd. <www.einfochips.com>
+ * Written-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
+ *
+ * (C) Copyright 2010
+ * Marvell Semiconductor <www.marvell.com>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <fdtdec.h>
+#include <pci_ids.h>
+#include <asm/bitops.h>
+#include <asm/gpio.h>
+#include <asm/io.h>
+#include <linux/bitfield.h>
+#include <linux/compat.h>
+#include <dt-bindings/gpio/gpio.h>
+
+/* Returns the bit value to write or read based on the offset */
+#define GPIO_BIT(x)		BIT_ULL((x) & 0x3f)
+
+#define GPIO_RX_DAT		0x00
+#define GPIO_TX_SET		0x08
+#define GPIO_TX_CLR		0x10
+#define GPIO_CONST		0x90	/* OcteonTX only */
+
+/* Offset to register-set for 2nd GPIOs (> 63), OcteonTX only */
+#define GPIO1_OFFSET		0x1400
+
+/* GPIO_CONST register bits */
+#define GPIO_CONST_GPIOS_MASK	GENMASK_ULL(7, 0)
+
+/* GPIO_BIT_CFG register bits */
+#define GPIO_BIT_CFG_TX_OE	BIT_ULL(0)
+#define GPIO_BIT_CFG_PIN_XOR	BIT_ULL(1)
+#define GPIO_BIT_CFG_INT_EN	BIT_ULL(2)
+#define GPIO_BIT_CFG_PIN_SEL_MASK GENMASK_ULL(26, 16)
+
+enum {
+	PROBE_PCI = 0,		/* PCI based probing */
+	PROBE_DT,		/* DT based probing */
+};
+
+struct octeon_gpio_data {
+	int probe;
+	u32 reg_offs;
+	u32 gpio_bit_cfg_offs;
+};
+
+struct octeon_gpio {
+	void __iomem *base;
+	const struct octeon_gpio_data *data;
+};
+
+/* Returns the offset to the output register based on the offset and value */
+static u32 gpio_tx_reg(int offset, int value)
+{
+	u32 ret;
+
+	ret = value ? GPIO_TX_SET : GPIO_TX_CLR;
+	if (offset > 63)
+		ret += GPIO1_OFFSET;
+
+	return ret;
+}
+
+/* Returns the offset to the input data register based on the offset */
+static u32 gpio_rx_dat_reg(int offset)
+{
+	u32 ret;
+
+	ret = GPIO_RX_DAT;
+	if (offset > 63)
+		ret += GPIO1_OFFSET;
+
+	return ret;
+}
+
+static int octeon_gpio_dir_input(struct udevice *dev, unsigned int offset)
+{
+	struct octeon_gpio *gpio = dev_get_priv(dev);
+
+	debug("%s(%s, %u)\n", __func__, dev->name, offset);
+	clrbits_64(gpio->base + gpio->data->gpio_bit_cfg_offs + 8 * offset,
+		   GPIO_BIT_CFG_TX_OE | GPIO_BIT_CFG_PIN_XOR |
+		   GPIO_BIT_CFG_INT_EN | GPIO_BIT_CFG_PIN_SEL_MASK);
+
+	return 0;
+}
+
+static int octeon_gpio_dir_output(struct udevice *dev, unsigned int offset,
+				  int value)
+{
+	struct octeon_gpio *gpio = dev_get_priv(dev);
+
+	debug("%s(%s, %u, %d)\n", __func__, dev->name, offset, value);
+	writeq(GPIO_BIT(offset), gpio->base + gpio->data->reg_offs +
+	       gpio_tx_reg(offset, value));
+
+	clrsetbits_64(gpio->base + gpio->data->gpio_bit_cfg_offs + 8 * offset,
+		      GPIO_BIT_CFG_PIN_SEL_MASK | GPIO_BIT_CFG_INT_EN,
+		      GPIO_BIT_CFG_TX_OE);
+
+	return 0;
+}
+
+static int octeon_gpio_get_value(struct udevice *dev, unsigned int offset)
+{
+	struct octeon_gpio *gpio = dev_get_priv(dev);
+	u64 reg = readq(gpio->base + gpio->data->reg_offs +
+			gpio_rx_dat_reg(offset));
+
+	debug("%s(%s, %u): value: %d\n", __func__, dev->name, offset,
+	      !!(reg & GPIO_BIT(offset)));
+
+	return !!(reg & GPIO_BIT(offset));
+}
+
+static int octeon_gpio_set_value(struct udevice *dev,
+				 unsigned int offset, int value)
+{
+	struct octeon_gpio *gpio = dev_get_priv(dev);
+
+	debug("%s(%s, %u, %d)\n", __func__, dev->name, offset, value);
+	writeq(GPIO_BIT(offset), gpio->base + gpio->data->reg_offs +
+	       gpio_tx_reg(offset, value));
+
+	return 0;
+}
+
+static int octeon_gpio_get_function(struct udevice *dev, unsigned int offset)
+{
+	struct octeon_gpio *gpio = dev_get_priv(dev);
+	u64 val = readq(gpio->base + gpio->data->gpio_bit_cfg_offs +
+			8 * offset);
+	int pin_sel;
+
+	debug("%s(%s, %u): GPIO_BIT_CFG: 0x%llx\n", __func__, dev->name,
+	      offset, val);
+	pin_sel = FIELD_GET(GPIO_BIT_CFG_PIN_SEL_MASK, val);
+	if (pin_sel)
+		return GPIOF_FUNC;
+	else if (val & GPIO_BIT_CFG_TX_OE)
+		return GPIOF_OUTPUT;
+	else
+		return GPIOF_INPUT;
+}
+
+static int octeon_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,
+			     struct ofnode_phandle_args *args)
+{
+	if (args->args_count < 1)
+		return -EINVAL;
+
+	desc->offset = args->args[0];
+	desc->flags = 0;
+	if (args->args_count > 1) {
+		if (args->args[1] & GPIO_ACTIVE_LOW)
+			desc->flags |= GPIOD_ACTIVE_LOW;
+		/* In the future add tri-state flag support */
+	}
+	return 0;
+}
+
+static const struct dm_gpio_ops octeon_gpio_ops = {
+	.direction_input	= octeon_gpio_dir_input,
+	.direction_output	= octeon_gpio_dir_output,
+	.get_value		= octeon_gpio_get_value,
+	.set_value		= octeon_gpio_set_value,
+	.get_function		= octeon_gpio_get_function,
+	.xlate			= octeon_gpio_xlate,
+};
+
+static int octeon_gpio_probe(struct udevice *dev)
+{
+	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+	struct octeon_gpio *priv = dev_get_priv(dev);
+	char *end;
+
+	priv->data = (const struct octeon_gpio_data *)dev_get_driver_data(dev);
+
+	if (priv->data->probe == PROBE_PCI) {
+		priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
+					    PCI_REGION_MEM);
+		uc_priv->gpio_count = readq(priv->base +
+					    priv->data->reg_offs + GPIO_CONST) &
+			GPIO_CONST_GPIOS_MASK;
+	} else {
+		priv->base = dev_remap_addr(dev);
+		uc_priv->gpio_count = ofnode_read_u32_default(dev->node,
+							      "nr-gpios", 32);
+	}
+
+	if (!priv->base) {
+		debug("%s(%s): Could not get base address\n",
+		      __func__, dev->name);
+		return -ENODEV;
+	}
+
+	uc_priv->bank_name  = strdup(dev->name);
+	end = strchr(uc_priv->bank_name, '@');
+	end[0] = 'A' + dev->seq;
+	end[1] = '\0';
+
+	debug("%s(%s): base address: %p, pin count: %d\n",
+	      __func__, dev->name, priv->base, uc_priv->gpio_count);
+
+	return 0;
+}
+
+static const struct octeon_gpio_data gpio_octeon_data = {
+	.probe = PROBE_DT,
+	.reg_offs = 0x80,
+	.gpio_bit_cfg_offs = 0x100,
+};
+
+static const struct octeon_gpio_data gpio_octeontx_data = {
+	.probe = PROBE_PCI,
+	.reg_offs = 0x00,
+	.gpio_bit_cfg_offs = 0x400,
+};
+
+static const struct udevice_id octeon_gpio_ids[] = {
+	{ .compatible = "cavium,thunder-8890-gpio",
+	  .data = (ulong)&gpio_octeontx_data },
+	{ .compatible = "cavium,octeon-7890-gpio",
+	  .data = (ulong)&gpio_octeon_data },
+	{ }
+};
+
+U_BOOT_DRIVER(octeon_gpio) = {
+	.name	= "octeon_gpio",
+	.id	= UCLASS_GPIO,
+	.of_match = of_match_ptr(octeon_gpio_ids),
+	.probe = octeon_gpio_probe,
+	.priv_auto_alloc_size = sizeof(struct octeon_gpio),
+	.ops	= &octeon_gpio_ops,
+	.flags	= DM_FLAG_PRE_RELOC,
+};
+
+static struct pci_device_id octeon_gpio_supported[] = {
+	{ PCI_VDEVICE(CAVIUM, PCI_DEVICE_ID_CAVIUM_GPIO),
+	  .driver_data = (ulong)&gpio_octeontx_data },
+	{ },
+};
+
+U_BOOT_PCI_DEVICE(octeon_gpio, octeon_gpio_supported);
-- 
2.26.2

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-05-26 12:19 [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon Stefan Roese
@ 2020-05-31 14:08 ` Simon Glass
  2020-07-16 18:44 ` Daniel Schwierzeck
  2020-07-18 13:25 ` Daniel Schwierzeck
  2 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2020-05-31 14:08 UTC (permalink / raw)
  To: u-boot

On Tue, 26 May 2020 at 06:19, Stefan Roese <sr@denx.de> wrote:
>
> From: Suneel Garapati <sgarapati@marvell.com>
>
> Add support for GPIO controllers found on Octeon II/III and Octeon TX
> TX2 SoC platforms.
>
> Signed-off-by: Aaron Williams <awilliams@marvell.com>
> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> Cc: Aaron Williams <awilliams@marvell.com>
> Cc: Chandrakala Chavva <cchavva@marvell.com>
> ---
> v2 (Stefan):
> - Removed #ifdef's for Octeon vs OcteonTX/TX2 completely
>   The differentiation is now made via driver data / compatible
>   string
>
> RFC -> v1 (Stefan)
> - Separated this patch from the OcteonTX/TX2 RFC patch series into a
>   single patch. This is useful, as the upcoming MIPS Octeon support will
>   use this GPIO driver.
> - Added MIPS Octeon II/III support (big endian). Rename driver and its
>   function names from "octeontx" to "octeon" to better match all Octeon
>   platforms.
> - Moved from union to defines / bitmasks. This makes the driver usage
>   on little- and big-endian platforms much easier.
> - Used clrbits_64() instead of clrbits_le64() and friends to support
>   usage on little- and big-endian systems
> - Removed dev->req_seq assignment
> - Enhanced Kconfig text
> - Rewrote GPIO_BIT macro
> - Dropped many macros to calculate the registers offsets and implemented
>   simple functions for this (easier to read)
> - Used GENMASK_ULL and FIELD_GET helpers
> - Minor cosmetic changes (dropped brackets etc)
> - Reword commit text and subject
>
>  drivers/gpio/Kconfig       |  10 ++
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/octeon_gpio.c | 253 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 264 insertions(+)
>  create mode 100644 drivers/gpio/octeon_gpio.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-05-26 12:19 [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon Stefan Roese
  2020-05-31 14:08 ` Simon Glass
@ 2020-07-16 18:44 ` Daniel Schwierzeck
  2020-07-17  6:03   ` Stefan Roese
  2020-07-18 13:25 ` Daniel Schwierzeck
  2 siblings, 1 reply; 11+ messages in thread
From: Daniel Schwierzeck @ 2020-07-16 18:44 UTC (permalink / raw)
  To: u-boot


> From: Suneel Garapati <sgarapati@marvell.com>
> 
> Add support for GPIO controllers found on Octeon II/III and Octeon TX
> TX2 SoC platforms.
> 
> Signed-off-by: Aaron Williams <awilliams@marvell.com>
> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> Cc: Aaron Williams <awilliams@marvell.com>
> Cc: Chandrakala Chavva <cchavva@marvell.com>
> ---
> v2 (Stefan):
> - Removed #ifdef's for Octeon vs OcteonTX/TX2 completely
>   The differentiation is now made via driver data / compatible
>   string
> 
> RFC -> v1 (Stefan)
> - Separated this patch from the OcteonTX/TX2 RFC patch series into a
>   single patch. This is useful, as the upcoming MIPS Octeon support will
>   use this GPIO driver.
> - Added MIPS Octeon II/III support (big endian). Rename driver and its
>   function names from "octeontx" to "octeon" to better match all Octeon
>   platforms.
> - Moved from union to defines / bitmasks. This makes the driver usage
>   on little- and big-endian platforms much easier.
> - Used clrbits_64() instead of clrbits_le64() and friends to support
>   usage on little- and big-endian systems
> - Removed dev->req_seq assignment
> - Enhanced Kconfig text
> - Rewrote GPIO_BIT macro
> - Dropped many macros to calculate the registers offsets and implemented
>   simple functions for this (easier to read)
> - Used GENMASK_ULL and FIELD_GET helpers
> - Minor cosmetic changes (dropped brackets etc)
> - Reword commit text and subject
> 
>  drivers/gpio/Kconfig       |  10 ++
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/octeon_gpio.c | 253 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 264 insertions(+)
>  create mode 100644 drivers/gpio/octeon_gpio.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index d87f6cc105..451896f400 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -316,6 +316,16 @@ config PIC32_GPIO
>  	help
>  	  Say yes here to support Microchip PIC32 GPIOs.
>  
> +config OCTEON_GPIO
> +	bool "Octeon II/III/TX/TX2 GPIO driver"
> +	depends on DM_GPIO && (ARCH_OCTEON || ARCH_OCTEONTX || ARCH_OCTEONTX2)
> +	default y
> +	help
> +	  Add support for the Marvell Octeon GPIO driver. This is used with
> +	  various Octeon parts such as Octeon II/III and OcteonTX/TX2.
> +	  Octeon II/III has 32 GPIOs (count defined via DT) and OcteonTX/TX2
> +	  has 64 GPIOs (count defined via internal register).
> +
>  config STM32_GPIO
>  	bool "ST STM32 GPIO driver"
>  	depends on DM_GPIO && (ARCH_STM32 || ARCH_STM32MP)
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 7638259007..eb6364adb4 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_HIKEY_GPIO)	+= hi6220_gpio.o
>  obj-$(CONFIG_HSDK_CREG_GPIO)	+= hsdk-creg-gpio.o
>  obj-$(CONFIG_IMX_RGPIO2P)	+= imx_rgpio2p.o
>  obj-$(CONFIG_PIC32_GPIO)	+= pic32_gpio.o
> +obj-$(CONFIG_OCTEON_GPIO)	+= octeon_gpio.o
>  obj-$(CONFIG_MVEBU_GPIO)	+= mvebu_gpio.o
>  obj-$(CONFIG_MSM_GPIO)		+= msm_gpio.o
>  obj-$(CONFIG_$(SPL_)PCF8575_GPIO)	+= pcf8575_gpio.o
> diff --git a/drivers/gpio/octeon_gpio.c b/drivers/gpio/octeon_gpio.c
> new file mode 100644
> index 0000000000..d7ac9a1910
> --- /dev/null
> +++ b/drivers/gpio/octeon_gpio.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier:    GPL-2.0
> +/*
> + * Copyright (C) 2018 Marvell International Ltd.
> + *
> + * (C) Copyright 2011
> + * eInfochips Ltd. <www.einfochips.com>
> + * Written-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> + *
> + * (C) Copyright 2010
> + * Marvell Semiconductor <www.marvell.com>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <fdtdec.h>
> +#include <pci_ids.h>
> +#include <asm/bitops.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <linux/bitfield.h>
> +#include <linux/compat.h>
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/* Returns the bit value to write or read based on the offset */
> +#define GPIO_BIT(x)		BIT_ULL((x) & 0x3f)
> +
> +#define GPIO_RX_DAT		0x00
> +#define GPIO_TX_SET		0x08
> +#define GPIO_TX_CLR		0x10
> +#define GPIO_CONST		0x90	/* OcteonTX only */
> +
> +/* Offset to register-set for 2nd GPIOs (> 63), OcteonTX only */
> +#define GPIO1_OFFSET		0x1400
> +
> +/* GPIO_CONST register bits */
> +#define GPIO_CONST_GPIOS_MASK	GENMASK_ULL(7, 0)
> +
> +/* GPIO_BIT_CFG register bits */
> +#define GPIO_BIT_CFG_TX_OE	BIT_ULL(0)
> +#define GPIO_BIT_CFG_PIN_XOR	BIT_ULL(1)
> +#define GPIO_BIT_CFG_INT_EN	BIT_ULL(2)
> +#define GPIO_BIT_CFG_PIN_SEL_MASK GENMASK_ULL(26, 16)
> +
> +enum {
> +	PROBE_PCI = 0,		/* PCI based probing */
> +	PROBE_DT,		/* DT based probing */
> +};
> +
> +struct octeon_gpio_data {
> +	int probe;
> +	u32 reg_offs;
> +	u32 gpio_bit_cfg_offs;
> +};
> +
> +struct octeon_gpio {
> +	void __iomem *base;
> +	const struct octeon_gpio_data *data;
> +};
> +
> +/* Returns the offset to the output register based on the offset and value */
> +static u32 gpio_tx_reg(int offset, int value)
> +{
> +	u32 ret;
> +
> +	ret = value ? GPIO_TX_SET : GPIO_TX_CLR;
> +	if (offset > 63)
> +		ret += GPIO1_OFFSET;
> +
> +	return ret;
> +}
> +
> +/* Returns the offset to the input data register based on the offset */
> +static u32 gpio_rx_dat_reg(int offset)
> +{
> +	u32 ret;
> +
> +	ret = GPIO_RX_DAT;
> +	if (offset > 63)
> +		ret += GPIO1_OFFSET;
> +
> +	return ret;
> +}
> +
> +static int octeon_gpio_dir_input(struct udevice *dev, unsigned int offset)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +
> +	debug("%s(%s, %u)\n", __func__, dev->name, offset);
> +	clrbits_64(gpio->base + gpio->data->gpio_bit_cfg_offs + 8 * offset,
> +		   GPIO_BIT_CFG_TX_OE | GPIO_BIT_CFG_PIN_XOR |
> +		   GPIO_BIT_CFG_INT_EN | GPIO_BIT_CFG_PIN_SEL_MASK);
> +
> +	return 0;
> +}
> +
> +static int octeon_gpio_dir_output(struct udevice *dev, unsigned int offset,
> +				  int value)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +
> +	debug("%s(%s, %u, %d)\n", __func__, dev->name, offset, value);
> +	writeq(GPIO_BIT(offset), gpio->base + gpio->data->reg_offs +
> +	       gpio_tx_reg(offset, value));
> +
> +	clrsetbits_64(gpio->base + gpio->data->gpio_bit_cfg_offs + 8 * offset,
> +		      GPIO_BIT_CFG_PIN_SEL_MASK | GPIO_BIT_CFG_INT_EN,
> +		      GPIO_BIT_CFG_TX_OE);
> +
> +	return 0;
> +}
> +
> +static int octeon_gpio_get_value(struct udevice *dev, unsigned int offset)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +	u64 reg = readq(gpio->base + gpio->data->reg_offs +
> +			gpio_rx_dat_reg(offset));
> +
> +	debug("%s(%s, %u): value: %d\n", __func__, dev->name, offset,
> +	      !!(reg & GPIO_BIT(offset)));
> +
> +	return !!(reg & GPIO_BIT(offset));
> +}
> +
> +static int octeon_gpio_set_value(struct udevice *dev,
> +				 unsigned int offset, int value)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +
> +	debug("%s(%s, %u, %d)\n", __func__, dev->name, offset, value);
> +	writeq(GPIO_BIT(offset), gpio->base + gpio->data->reg_offs +
> +	       gpio_tx_reg(offset, value));
> +
> +	return 0;
> +}
> +
> +static int octeon_gpio_get_function(struct udevice *dev, unsigned int offset)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +	u64 val = readq(gpio->base + gpio->data->gpio_bit_cfg_offs +
> +			8 * offset);
> +	int pin_sel;
> +
> +	debug("%s(%s, %u): GPIO_BIT_CFG: 0x%llx\n", __func__, dev->name,
> +	      offset, val);
> +	pin_sel = FIELD_GET(GPIO_BIT_CFG_PIN_SEL_MASK, val);
> +	if (pin_sel)
> +		return GPIOF_FUNC;
> +	else if (val & GPIO_BIT_CFG_TX_OE)
> +		return GPIOF_OUTPUT;
> +	else
> +		return GPIOF_INPUT;
> +}
> +
> +static int octeon_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,
> +			     struct ofnode_phandle_args *args)
> +{
> +	if (args->args_count < 1)
> +		return -EINVAL;
> +
> +	desc->offset = args->args[0];
> +	desc->flags = 0;
> +	if (args->args_count > 1) {
> +		if (args->args[1] & GPIO_ACTIVE_LOW)
> +			desc->flags |= GPIOD_ACTIVE_LOW;
> +		/* In the future add tri-state flag support */
> +	}
> +	return 0;
> +}
> +
> +static const struct dm_gpio_ops octeon_gpio_ops = {
> +	.direction_input	= octeon_gpio_dir_input,
> +	.direction_output	= octeon_gpio_dir_output,
> +	.get_value		= octeon_gpio_get_value,
> +	.set_value		= octeon_gpio_set_value,
> +	.get_function		= octeon_gpio_get_function,
> +	.xlate			= octeon_gpio_xlate,
> +};
> +
> +static int octeon_gpio_probe(struct udevice *dev)
> +{
> +	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +	struct octeon_gpio *priv = dev_get_priv(dev);
> +	char *end;
> +
> +	priv->data = (const struct octeon_gpio_data *)dev_get_driver_data(dev);
> +
> +	if (priv->data->probe == PROBE_PCI) {
> +		priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> +					    PCI_REGION_MEM);
> +		uc_priv->gpio_count = readq(priv->base +
> +					    priv->data->reg_offs + GPIO_CONST) &
> +			GPIO_CONST_GPIOS_MASK;
> +	} else {
> +		priv->base = dev_remap_addr(dev);
> +		uc_priv->gpio_count = ofnode_read_u32_default(dev->node,
> +							      "nr-gpios", 32);
> +	}
> +
> +	if (!priv->base) {
> +		debug("%s(%s): Could not get base address\n",
> +		      __func__, dev->name);
> +		return -ENODEV;
> +	}
> +
> +	uc_priv->bank_name  = strdup(dev->name);
> +	end = strchr(uc_priv->bank_name, '@');
> +	end[0] = 'A' + dev->seq;
> +	end[1] = '\0';
> +
> +	debug("%s(%s): base address: %p, pin count: %d\n",
> +	      __func__, dev->name, priv->base, uc_priv->gpio_count);
> +
> +	return 0;
> +}
> +
> +static const struct octeon_gpio_data gpio_octeon_data = {
> +	.probe = PROBE_DT,
> +	.reg_offs = 0x80,
> +	.gpio_bit_cfg_offs = 0x100,
> +};
> +
> +static const struct octeon_gpio_data gpio_octeontx_data = {
> +	.probe = PROBE_PCI,
> +	.reg_offs = 0x00,
> +	.gpio_bit_cfg_offs = 0x400,
> +};
> +
> +static const struct udevice_id octeon_gpio_ids[] = {
> +	{ .compatible = "cavium,thunder-8890-gpio",
> +	  .data = (ulong)&gpio_octeontx_data },
> +	{ .compatible = "cavium,octeon-7890-gpio",
> +	  .data = (ulong)&gpio_octeon_data },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(octeon_gpio) = {
> +	.name	= "octeon_gpio",
> +	.id	= UCLASS_GPIO,
> +	.of_match = of_match_ptr(octeon_gpio_ids),
> +	.probe = octeon_gpio_probe,
> +	.priv_auto_alloc_size = sizeof(struct octeon_gpio),
> +	.ops	= &octeon_gpio_ops,
> +	.flags	= DM_FLAG_PRE_RELOC,
> +};
> +
> +static struct pci_device_id octeon_gpio_supported[] = {
> +	{ PCI_VDEVICE(CAVIUM, PCI_DEVICE_ID_CAVIUM_GPIO),
> +	  .driver_data = (ulong)&gpio_octeontx_data },
> +	{ },
> +};

I can't build that with the Octeon base support because
PCI_VENDOR_ID_CAVIUM and PCI_DEVICE_ID_CAVIUM_GPIO are missing in U-
Boot's include/pci_ids.h. I can only find PCI_VENDOR_ID_CAVIUM in Linux
but not PCI_DEVICE_ID_CAVIUM_GPIO.

> +
> +U_BOOT_PCI_DEVICE(octeon_gpio, octeon_gpio_supported);
-- 
- Daniel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-07-16 18:44 ` Daniel Schwierzeck
@ 2020-07-17  6:03   ` Stefan Roese
  0 siblings, 0 replies; 11+ messages in thread
From: Stefan Roese @ 2020-07-17  6:03 UTC (permalink / raw)
  To: u-boot

Hi Daniel,

On 16.07.20 20:44, Daniel Schwierzeck wrote:

<snip>

>> +U_BOOT_DRIVER(octeon_gpio) = {
>> +	.name	= "octeon_gpio",
>> +	.id	= UCLASS_GPIO,
>> +	.of_match = of_match_ptr(octeon_gpio_ids),
>> +	.probe = octeon_gpio_probe,
>> +	.priv_auto_alloc_size = sizeof(struct octeon_gpio),
>> +	.ops	= &octeon_gpio_ops,
>> +	.flags	= DM_FLAG_PRE_RELOC,
>> +};
>> +
>> +static struct pci_device_id octeon_gpio_supported[] = {
>> +	{ PCI_VDEVICE(CAVIUM, PCI_DEVICE_ID_CAVIUM_GPIO),
>> +	  .driver_data = (ulong)&gpio_octeontx_data },
>> +	{ },
>> +};
> 
> I can't build that with the Octeon base support because
> PCI_VENDOR_ID_CAVIUM and PCI_DEVICE_ID_CAVIUM_GPIO are missing in U-
> Boot's include/pci_ids.h. I can only find PCI_VENDOR_ID_CAVIUM in Linux
> but not PCI_DEVICE_ID_CAVIUM_GPIO.

Ah yes. Sorry, I missed sending this pci_ids.h patch that I have applied
in my local branches. Here a link to the patch, which has been updated
since the first RFC version:

https://www.mail-archive.com/u-boot at lists.denx.de/msg345960.html

I'll send the new version in a few minutes.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-05-26 12:19 [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon Stefan Roese
  2020-05-31 14:08 ` Simon Glass
  2020-07-16 18:44 ` Daniel Schwierzeck
@ 2020-07-18 13:25 ` Daniel Schwierzeck
  2020-07-21 11:03   ` Stefan Roese
  2 siblings, 1 reply; 11+ messages in thread
From: Daniel Schwierzeck @ 2020-07-18 13:25 UTC (permalink / raw)
  To: u-boot


> From: Suneel Garapati <sgarapati@marvell.com>
> 
> Add support for GPIO controllers found on Octeon II/III and Octeon TX
> TX2 SoC platforms.
> 
> Signed-off-by: Aaron Williams <awilliams@marvell.com>
> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> Cc: Aaron Williams <awilliams@marvell.com>
> Cc: Chandrakala Chavva <cchavva@marvell.com>
> ---
> v2 (Stefan):
> - Removed #ifdef's for Octeon vs OcteonTX/TX2 completely
>   The differentiation is now made via driver data / compatible
>   string
> 
> RFC -> v1 (Stefan)
> - Separated this patch from the OcteonTX/TX2 RFC patch series into a
>   single patch. This is useful, as the upcoming MIPS Octeon support will
>   use this GPIO driver.
> - Added MIPS Octeon II/III support (big endian). Rename driver and its
>   function names from "octeontx" to "octeon" to better match all Octeon
>   platforms.
> - Moved from union to defines / bitmasks. This makes the driver usage
>   on little- and big-endian platforms much easier.
> - Used clrbits_64() instead of clrbits_le64() and friends to support
>   usage on little- and big-endian systems
> - Removed dev->req_seq assignment
> - Enhanced Kconfig text
> - Rewrote GPIO_BIT macro
> - Dropped many macros to calculate the registers offsets and implemented
>   simple functions for this (easier to read)
> - Used GENMASK_ULL and FIELD_GET helpers
> - Minor cosmetic changes (dropped brackets etc)
> - Reword commit text and subject
> 
>  drivers/gpio/Kconfig       |  10 ++
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/octeon_gpio.c | 253 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 264 insertions(+)
>  create mode 100644 drivers/gpio/octeon_gpio.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index d87f6cc105..451896f400 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -316,6 +316,16 @@ config PIC32_GPIO
>  	help
>  	  Say yes here to support Microchip PIC32 GPIOs.
>  
> +config OCTEON_GPIO
> +	bool "Octeon II/III/TX/TX2 GPIO driver"
> +	depends on DM_GPIO && (ARCH_OCTEON || ARCH_OCTEONTX || ARCH_OCTEONTX2)
> +	default y
> +	help
> +	  Add support for the Marvell Octeon GPIO driver. This is used with
> +	  various Octeon parts such as Octeon II/III and OcteonTX/TX2.
> +	  Octeon II/III has 32 GPIOs (count defined via DT) and OcteonTX/TX2
> +	  has 64 GPIOs (count defined via internal register).
> +

found another issue:

drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
function-declaration]
  189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
      |                ^~~~~~~~~~~~~~
      |                pci_map_bar
drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
'int' makes pointer from integer without a cast [-Wint-conversion]
  189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
      |              ^

due to the dependency on DM_PCI you need to express this in Kconfig
with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
function prototypes in pci.h are also wrapped with "#ifdef
CONFIG_DM_PCI". The former option will build and link DM PCI code which
is not used and therefore bloats the U-Boot binary.

I guess the choice mainly depends on whether you'll add a PCI host
controller driver for Octeon MIPS64 in the future and can live with the
extra but unused PCI code until then.

>  config STM32_GPIO
>  	bool "ST STM32 GPIO driver"
>  	depends on DM_GPIO && (ARCH_STM32 || ARCH_STM32MP)
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 7638259007..eb6364adb4 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_HIKEY_GPIO)	+= hi6220_gpio.o
>  obj-$(CONFIG_HSDK_CREG_GPIO)	+= hsdk-creg-gpio.o
>  obj-$(CONFIG_IMX_RGPIO2P)	+= imx_rgpio2p.o
>  obj-$(CONFIG_PIC32_GPIO)	+= pic32_gpio.o
> +obj-$(CONFIG_OCTEON_GPIO)	+= octeon_gpio.o
>  obj-$(CONFIG_MVEBU_GPIO)	+= mvebu_gpio.o
>  obj-$(CONFIG_MSM_GPIO)		+= msm_gpio.o
>  obj-$(CONFIG_$(SPL_)PCF8575_GPIO)	+= pcf8575_gpio.o
> diff --git a/drivers/gpio/octeon_gpio.c b/drivers/gpio/octeon_gpio.c
> new file mode 100644
> index 0000000000..d7ac9a1910
> --- /dev/null
> +++ b/drivers/gpio/octeon_gpio.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier:    GPL-2.0
> +/*
> + * Copyright (C) 2018 Marvell International Ltd.
> + *
> + * (C) Copyright 2011
> + * eInfochips Ltd. <www.einfochips.com>
> + * Written-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> + *
> + * (C) Copyright 2010
> + * Marvell Semiconductor <www.marvell.com>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <fdtdec.h>
> +#include <pci_ids.h>
> +#include <asm/bitops.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <linux/bitfield.h>
> +#include <linux/compat.h>
> +#include <dt-bindings/gpio/gpio.h>

don't use common.h anymore. Also some headers are unused. I can build
the driver with:

#include <dm.h>
#include <pci.h>
#include <pci_ids.h>
#include <asm/gpio.h>
#include <asm/io.h>
#include <linux/bitfield.h>
#include <linux/compat.h>
#include <dt-bindings/gpio/gpio.h>

> +
> +/* Returns the bit value to write or read based on the offset */
> +#define GPIO_BIT(x)		BIT_ULL((x) & 0x3f)
> +
> +#define GPIO_RX_DAT		0x00
> +#define GPIO_TX_SET		0x08
> +#define GPIO_TX_CLR		0x10
> +#define GPIO_CONST		0x90	/* OcteonTX only */
> +
> +/* Offset to register-set for 2nd GPIOs (> 63), OcteonTX only */
> +#define GPIO1_OFFSET		0x1400
> +
> +/* GPIO_CONST register bits */
> +#define GPIO_CONST_GPIOS_MASK	GENMASK_ULL(7, 0)
> +
> +/* GPIO_BIT_CFG register bits */
> +#define GPIO_BIT_CFG_TX_OE	BIT_ULL(0)
> +#define GPIO_BIT_CFG_PIN_XOR	BIT_ULL(1)
> +#define GPIO_BIT_CFG_INT_EN	BIT_ULL(2)
> +#define GPIO_BIT_CFG_PIN_SEL_MASK GENMASK_ULL(26, 16)
> +
> +enum {
> +	PROBE_PCI = 0,		/* PCI based probing */
> +	PROBE_DT,		/* DT based probing */
> +};
> +
> +struct octeon_gpio_data {
> +	int probe;
> +	u32 reg_offs;
> +	u32 gpio_bit_cfg_offs;
> +};
> +
> +struct octeon_gpio {
> +	void __iomem *base;
> +	const struct octeon_gpio_data *data;
> +};
> +
> +/* Returns the offset to the output register based on the offset and value */
> +static u32 gpio_tx_reg(int offset, int value)
> +{
> +	u32 ret;
> +
> +	ret = value ? GPIO_TX_SET : GPIO_TX_CLR;
> +	if (offset > 63)
> +		ret += GPIO1_OFFSET;
> +
> +	return ret;
> +}
> +
> +/* Returns the offset to the input data register based on the offset */
> +static u32 gpio_rx_dat_reg(int offset)
> +{
> +	u32 ret;
> +
> +	ret = GPIO_RX_DAT;
> +	if (offset > 63)
> +		ret += GPIO1_OFFSET;
> +
> +	return ret;
> +}
> +
> +static int octeon_gpio_dir_input(struct udevice *dev, unsigned int offset)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +
> +	debug("%s(%s, %u)\n", __func__, dev->name, offset);
> +	clrbits_64(gpio->base + gpio->data->gpio_bit_cfg_offs + 8 * offset,
> +		   GPIO_BIT_CFG_TX_OE | GPIO_BIT_CFG_PIN_XOR |
> +		   GPIO_BIT_CFG_INT_EN | GPIO_BIT_CFG_PIN_SEL_MASK);
> +
> +	return 0;
> +}
> +
> +static int octeon_gpio_dir_output(struct udevice *dev, unsigned int offset,
> +				  int value)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +
> +	debug("%s(%s, %u, %d)\n", __func__, dev->name, offset, value);
> +	writeq(GPIO_BIT(offset), gpio->base + gpio->data->reg_offs +
> +	       gpio_tx_reg(offset, value));
> +
> +	clrsetbits_64(gpio->base + gpio->data->gpio_bit_cfg_offs + 8 * offset,
> +		      GPIO_BIT_CFG_PIN_SEL_MASK | GPIO_BIT_CFG_INT_EN,
> +		      GPIO_BIT_CFG_TX_OE);
> +
> +	return 0;
> +}
> +
> +static int octeon_gpio_get_value(struct udevice *dev, unsigned int offset)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +	u64 reg = readq(gpio->base + gpio->data->reg_offs +
> +			gpio_rx_dat_reg(offset));
> +
> +	debug("%s(%s, %u): value: %d\n", __func__, dev->name, offset,
> +	      !!(reg & GPIO_BIT(offset)));
> +
> +	return !!(reg & GPIO_BIT(offset));
> +}
> +
> +static int octeon_gpio_set_value(struct udevice *dev,
> +				 unsigned int offset, int value)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +
> +	debug("%s(%s, %u, %d)\n", __func__, dev->name, offset, value);
> +	writeq(GPIO_BIT(offset), gpio->base + gpio->data->reg_offs +
> +	       gpio_tx_reg(offset, value));
> +
> +	return 0;
> +}
> +
> +static int octeon_gpio_get_function(struct udevice *dev, unsigned int offset)
> +{
> +	struct octeon_gpio *gpio = dev_get_priv(dev);
> +	u64 val = readq(gpio->base + gpio->data->gpio_bit_cfg_offs +
> +			8 * offset);
> +	int pin_sel;
> +
> +	debug("%s(%s, %u): GPIO_BIT_CFG: 0x%llx\n", __func__, dev->name,
> +	      offset, val);
> +	pin_sel = FIELD_GET(GPIO_BIT_CFG_PIN_SEL_MASK, val);
> +	if (pin_sel)
> +		return GPIOF_FUNC;
> +	else if (val & GPIO_BIT_CFG_TX_OE)
> +		return GPIOF_OUTPUT;
> +	else
> +		return GPIOF_INPUT;
> +}
> +
> +static int octeon_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,
> +			     struct ofnode_phandle_args *args)
> +{
> +	if (args->args_count < 1)
> +		return -EINVAL;
> +
> +	desc->offset = args->args[0];
> +	desc->flags = 0;
> +	if (args->args_count > 1) {
> +		if (args->args[1] & GPIO_ACTIVE_LOW)
> +			desc->flags |= GPIOD_ACTIVE_LOW;
> +		/* In the future add tri-state flag support */
> +	}
> +	return 0;
> +}
> +
> +static const struct dm_gpio_ops octeon_gpio_ops = {
> +	.direction_input	= octeon_gpio_dir_input,
> +	.direction_output	= octeon_gpio_dir_output,
> +	.get_value		= octeon_gpio_get_value,
> +	.set_value		= octeon_gpio_set_value,
> +	.get_function		= octeon_gpio_get_function,
> +	.xlate			= octeon_gpio_xlate,
> +};
> +
> +static int octeon_gpio_probe(struct udevice *dev)
> +{
> +	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +	struct octeon_gpio *priv = dev_get_priv(dev);
> +	char *end;
> +
> +	priv->data = (const struct octeon_gpio_data *)dev_get_driver_data(dev);
> +
> +	if (priv->data->probe == PROBE_PCI) {
> +		priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> +					    PCI_REGION_MEM);
> +		uc_priv->gpio_count = readq(priv->base +
> +					    priv->data->reg_offs + GPIO_CONST) &
> +			GPIO_CONST_GPIOS_MASK;
> +	} else {
> +		priv->base = dev_remap_addr(dev);
> +		uc_priv->gpio_count = ofnode_read_u32_default(dev->node,
> +							      "nr-gpios", 32);
> +	}
> +
> +	if (!priv->base) {
> +		debug("%s(%s): Could not get base address\n",
> +		      __func__, dev->name);
> +		return -ENODEV;
> +	}
> +
> +	uc_priv->bank_name  = strdup(dev->name);
> +	end = strchr(uc_priv->bank_name, '@');
> +	end[0] = 'A' + dev->seq;
> +	end[1] = '\0';
> +
> +	debug("%s(%s): base address: %p, pin count: %d\n",
> +	      __func__, dev->name, priv->base, uc_priv->gpio_count);
> +
> +	return 0;
> +}
> +
> +static const struct octeon_gpio_data gpio_octeon_data = {
> +	.probe = PROBE_DT,
> +	.reg_offs = 0x80,
> +	.gpio_bit_cfg_offs = 0x100,
> +};
> +
> +static const struct octeon_gpio_data gpio_octeontx_data = {
> +	.probe = PROBE_PCI,
> +	.reg_offs = 0x00,
> +	.gpio_bit_cfg_offs = 0x400,
> +};
> +
> +static const struct udevice_id octeon_gpio_ids[] = {
> +	{ .compatible = "cavium,thunder-8890-gpio",
> +	  .data = (ulong)&gpio_octeontx_data },
> +	{ .compatible = "cavium,octeon-7890-gpio",
> +	  .data = (ulong)&gpio_octeon_data },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(octeon_gpio) = {
> +	.name	= "octeon_gpio",
> +	.id	= UCLASS_GPIO,
> +	.of_match = of_match_ptr(octeon_gpio_ids),
> +	.probe = octeon_gpio_probe,
> +	.priv_auto_alloc_size = sizeof(struct octeon_gpio),
> +	.ops	= &octeon_gpio_ops,
> +	.flags	= DM_FLAG_PRE_RELOC,
> +};
> +
> +static struct pci_device_id octeon_gpio_supported[] = {
> +	{ PCI_VDEVICE(CAVIUM, PCI_DEVICE_ID_CAVIUM_GPIO),
> +	  .driver_data = (ulong)&gpio_octeontx_data },
> +	{ },
> +};
> +
> +U_BOOT_PCI_DEVICE(octeon_gpio, octeon_gpio_supported);
-- 
- Daniel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-07-18 13:25 ` Daniel Schwierzeck
@ 2020-07-21 11:03   ` Stefan Roese
  2020-07-21 14:59     ` Daniel Schwierzeck
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Roese @ 2020-07-21 11:03 UTC (permalink / raw)
  To: u-boot

Hi Daniel,

On 18.07.20 15:25, Daniel Schwierzeck wrote:
> 
>> From: Suneel Garapati <sgarapati@marvell.com>
>>
>> Add support for GPIO controllers found on Octeon II/III and Octeon TX
>> TX2 SoC platforms.
>>
>> Signed-off-by: Aaron Williams <awilliams@marvell.com>
>> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>> Cc: Aaron Williams <awilliams@marvell.com>
>> Cc: Chandrakala Chavva <cchavva@marvell.com>
>> ---
>> v2 (Stefan):
>> - Removed #ifdef's for Octeon vs OcteonTX/TX2 completely
>>    The differentiation is now made via driver data / compatible
>>    string
>>
>> RFC -> v1 (Stefan)
>> - Separated this patch from the OcteonTX/TX2 RFC patch series into a
>>    single patch. This is useful, as the upcoming MIPS Octeon support will
>>    use this GPIO driver.
>> - Added MIPS Octeon II/III support (big endian). Rename driver and its
>>    function names from "octeontx" to "octeon" to better match all Octeon
>>    platforms.
>> - Moved from union to defines / bitmasks. This makes the driver usage
>>    on little- and big-endian platforms much easier.
>> - Used clrbits_64() instead of clrbits_le64() and friends to support
>>    usage on little- and big-endian systems
>> - Removed dev->req_seq assignment
>> - Enhanced Kconfig text
>> - Rewrote GPIO_BIT macro
>> - Dropped many macros to calculate the registers offsets and implemented
>>    simple functions for this (easier to read)
>> - Used GENMASK_ULL and FIELD_GET helpers
>> - Minor cosmetic changes (dropped brackets etc)
>> - Reword commit text and subject
>>
>>   drivers/gpio/Kconfig       |  10 ++
>>   drivers/gpio/Makefile      |   1 +
>>   drivers/gpio/octeon_gpio.c | 253 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 264 insertions(+)
>>   create mode 100644 drivers/gpio/octeon_gpio.c
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index d87f6cc105..451896f400 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -316,6 +316,16 @@ config PIC32_GPIO
>>   	help
>>   	  Say yes here to support Microchip PIC32 GPIOs.
>>   
>> +config OCTEON_GPIO
>> +	bool "Octeon II/III/TX/TX2 GPIO driver"
>> +	depends on DM_GPIO && (ARCH_OCTEON || ARCH_OCTEONTX || ARCH_OCTEONTX2)
>> +	default y
>> +	help
>> +	  Add support for the Marvell Octeon GPIO driver. This is used with
>> +	  various Octeon parts such as Octeon II/III and OcteonTX/TX2.
>> +	  Octeon II/III has 32 GPIOs (count defined via DT) and OcteonTX/TX2
>> +	  has 64 GPIOs (count defined via internal register).
>> +
> 
> found another issue:
> 
> drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
> drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
> function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
> function-declaration]
>    189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
>        |                ^~~~~~~~~~~~~~
>        |                pci_map_bar
> drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
> 'int' makes pointer from integer without a cast [-Wint-conversion]
>    189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
>        |              ^

Ah, I did not see this in my local builds, as I have enabled DM_PCI
here and forgot to add this dependency.

> due to the dependency on DM_PCI you need to express this in Kconfig
> with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
> specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
> function prototypes in pci.h are also wrapped with "#ifdef
> CONFIG_DM_PCI". The former option will build and link DM PCI code which
> is not used and therefore bloats the U-Boot binary.
> 
> I guess the choice mainly depends on whether you'll add a PCI host
> controller driver for Octeon MIPS64 in the future and can live with the
> extra but unused PCI code until then.

We can definitely live with the temporarily unused PCI code. I don't
want to add these #ifdefs again, which I removed explicitly upon Simon's
request.

To solve this, I would prefer to add a "select DM_PCI & PCI" to
arch/mips/Kconfig for Octeon, as this PCI probing construct is not
only used in this GPIO driver, but also in many other drivers shared
between MIPS Octeon and the also upcoming ARM64 Octeon TX/TX2 support,
like I2C, SPI etc. Here all devices are PCI based hence the PCI probing
dependency.

Is this okay with you? Or should I stay with the dependency in the
drivers Kconfig file?

>>   config STM32_GPIO
>>   	bool "ST STM32 GPIO driver"
>>   	depends on DM_GPIO && (ARCH_STM32 || ARCH_STM32MP)
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 7638259007..eb6364adb4 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_HIKEY_GPIO)	+= hi6220_gpio.o
>>   obj-$(CONFIG_HSDK_CREG_GPIO)	+= hsdk-creg-gpio.o
>>   obj-$(CONFIG_IMX_RGPIO2P)	+= imx_rgpio2p.o
>>   obj-$(CONFIG_PIC32_GPIO)	+= pic32_gpio.o
>> +obj-$(CONFIG_OCTEON_GPIO)	+= octeon_gpio.o
>>   obj-$(CONFIG_MVEBU_GPIO)	+= mvebu_gpio.o
>>   obj-$(CONFIG_MSM_GPIO)		+= msm_gpio.o
>>   obj-$(CONFIG_$(SPL_)PCF8575_GPIO)	+= pcf8575_gpio.o
>> diff --git a/drivers/gpio/octeon_gpio.c b/drivers/gpio/octeon_gpio.c
>> new file mode 100644
>> index 0000000000..d7ac9a1910
>> --- /dev/null
>> +++ b/drivers/gpio/octeon_gpio.c
>> @@ -0,0 +1,253 @@
>> +// SPDX-License-Identifier:    GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Marvell International Ltd.
>> + *
>> + * (C) Copyright 2011
>> + * eInfochips Ltd. <www.einfochips.com>
>> + * Written-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
>> + *
>> + * (C) Copyright 2010
>> + * Marvell Semiconductor <www.marvell.com>
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <fdtdec.h>
>> +#include <pci_ids.h>
>> +#include <asm/bitops.h>
>> +#include <asm/gpio.h>
>> +#include <asm/io.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/compat.h>
>> +#include <dt-bindings/gpio/gpio.h>
> 
> don't use common.h anymore. Also some headers are unused. I can build
> the driver with:
> 
> #include <dm.h>
> #include <pci.h>
> #include <pci_ids.h>
> #include <asm/gpio.h>
> #include <asm/io.h>
> #include <linux/bitfield.h>
> #include <linux/compat.h>
> #include <dt-bindings/gpio/gpio.h>

Yes, thanks. I'll change this in v3.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-07-21 11:03   ` Stefan Roese
@ 2020-07-21 14:59     ` Daniel Schwierzeck
  2020-07-22  8:54       ` Stefan Roese
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Schwierzeck @ 2020-07-21 14:59 UTC (permalink / raw)
  To: u-boot


> Hi Daniel,
> 
> On 18.07.20 15:25, Daniel Schwierzeck wrote:
> > > From: Suneel Garapati <sgarapati@marvell.com>
...
> > found another issue:
> > 
> > drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
> > drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
> > function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
> > function-declaration]
> >    189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> >        |                ^~~~~~~~~~~~~~
> >        |                pci_map_bar
> > drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
> > 'int' makes pointer from integer without a cast [-Wint-conversion]
> >    189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> >        |              ^
> 
> Ah, I did not see this in my local builds, as I have enabled DM_PCI
> here and forgot to add this dependency.
> 
> > due to the dependency on DM_PCI you need to express this in Kconfig
> > with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
> > specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
> > function prototypes in pci.h are also wrapped with "#ifdef
> > CONFIG_DM_PCI". The former option will build and link DM PCI code which
> > is not used and therefore bloats the U-Boot binary.
> > 
> > I guess the choice mainly depends on whether you'll add a PCI host
> > controller driver for Octeon MIPS64 in the future and can live with the
> > extra but unused PCI code until then.
> 
> We can definitely live with the temporarily unused PCI code. I don't
> want to add these #ifdefs again, which I removed explicitly upon Simon's
> request.
> 
> To solve this, I would prefer to add a "select DM_PCI & PCI" to
> arch/mips/Kconfig for Octeon, as this PCI probing construct is not
> only used in this GPIO driver, but also in many other drivers shared
> between MIPS Octeon and the also upcoming ARM64 Octeon TX/TX2 support,
> like I2C, SPI etc. Here all devices are PCI based hence the PCI probing
> dependency.
> 
> Is this okay with you? Or should I stay with the dependency in the
> drivers Kconfig file?
> 

I would also prefer a "select DM_PCI". I just struggle a bit with the
"& PCI" part. IMHO DM_PCI should not depend on PCI because DM_PCI
should be a hidden option which is selected by the SoC dependent on the
SoC specific drivers and whether them support DM. It doesn't make sense
to let the user choose all those DM_* options. Most other DM_* options
already work like this except that them are not hidden.

BTW: When you prepare a patch, you can also "select DM_I2C" and
"DM_SPI" which have the same issues with the PCI dependency.

-- 
- Daniel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-07-21 14:59     ` Daniel Schwierzeck
@ 2020-07-22  8:54       ` Stefan Roese
  2020-07-22 11:47         ` Daniel Schwierzeck
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Roese @ 2020-07-22  8:54 UTC (permalink / raw)
  To: u-boot

Hi Daniel,

On 21.07.20 16:59, Daniel Schwierzeck wrote:
> 
>> Hi Daniel,
>>
>> On 18.07.20 15:25, Daniel Schwierzeck wrote:
>>>> From: Suneel Garapati <sgarapati@marvell.com>
> ...
>>> found another issue:
>>>
>>> drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
>>> drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
>>> function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
>>> function-declaration]
>>>     189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
>>>         |                ^~~~~~~~~~~~~~
>>>         |                pci_map_bar
>>> drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
>>> 'int' makes pointer from integer without a cast [-Wint-conversion]
>>>     189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
>>>         |              ^
>>
>> Ah, I did not see this in my local builds, as I have enabled DM_PCI
>> here and forgot to add this dependency.
>>
>>> due to the dependency on DM_PCI you need to express this in Kconfig
>>> with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
>>> specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
>>> function prototypes in pci.h are also wrapped with "#ifdef
>>> CONFIG_DM_PCI". The former option will build and link DM PCI code which
>>> is not used and therefore bloats the U-Boot binary.
>>>
>>> I guess the choice mainly depends on whether you'll add a PCI host
>>> controller driver for Octeon MIPS64 in the future and can live with the
>>> extra but unused PCI code until then.
>>
>> We can definitely live with the temporarily unused PCI code. I don't
>> want to add these #ifdefs again, which I removed explicitly upon Simon's
>> request.
>>
>> To solve this, I would prefer to add a "select DM_PCI & PCI" to
>> arch/mips/Kconfig for Octeon, as this PCI probing construct is not
>> only used in this GPIO driver, but also in many other drivers shared
>> between MIPS Octeon and the also upcoming ARM64 Octeon TX/TX2 support,
>> like I2C, SPI etc. Here all devices are PCI based hence the PCI probing
>> dependency.
>>
>> Is this okay with you? Or should I stay with the dependency in the
>> drivers Kconfig file?
>>
> 
> I would also prefer a "select DM_PCI". I just struggle a bit with the
> "& PCI" part. IMHO DM_PCI should not depend on PCI because DM_PCI
> should be a hidden option which is selected by the SoC dependent on the
> SoC specific drivers and whether them support DM. It doesn't make sense
> to let the user choose all those DM_* options. Most other DM_* options
> already work like this except that them are not hidden.
> 
> BTW: When you prepare a patch, you can also "select DM_I2C" and
> "DM_SPI" which have the same issues with the PCI dependency.

Sure. But selecting only "DM_PCI" and not "PCI" leads to these issues:

WARNING: unmet direct dependencies detected for DM_PCI
   Depends on [n]: PCI [=n] && DM [=y]
   Selected by [y]:
   - ARCH_OCTEON [=y] && <choice>

It doesn't make sense to have DM_PCI enabled and PCI not. Same happens
with DM_SPI BTW.

How would you like me to solve this? Enable PCI (and SPI etc) in the
board defconfig file? This will lead to problems with git-bisecting
though, as the Kconfig change depends on the defconfig change and
vice versa. I'll squash the Kconfig and defconfig updates in one
patch because of this.

I'll combine all these changes including the GPIO driver update, all
the dts updates and a newly introduced simple clk driver in a new
patchset shortly.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-07-22  8:54       ` Stefan Roese
@ 2020-07-22 11:47         ` Daniel Schwierzeck
  2020-07-23 10:07           ` Stefan Roese
  2020-07-28 18:58           ` Simon Glass
  0 siblings, 2 replies; 11+ messages in thread
From: Daniel Schwierzeck @ 2020-07-22 11:47 UTC (permalink / raw)
  To: u-boot

> Hi Daniel,
> 
> On 21.07.20 16:59, Daniel Schwierzeck wrote:
> > > Hi Daniel,
> > > 
> > > On 18.07.20 15:25, Daniel Schwierzeck wrote:
> > > > > From: Suneel Garapati <sgarapati@marvell.com>
> > ...
> > > > found another issue:
> > > > 
> > > > drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
> > > > drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
> > > > function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
> > > > function-declaration]
> > > >     189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> > > >         |                ^~~~~~~~~~~~~~
> > > >         |                pci_map_bar
> > > > drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
> > > > 'int' makes pointer from integer without a cast [-Wint-conversion]
> > > >     189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> > > >         |              ^
> > > 
> > > Ah, I did not see this in my local builds, as I have enabled DM_PCI
> > > here and forgot to add this dependency.
> > > 
> > > > due to the dependency on DM_PCI you need to express this in Kconfig
> > > > with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
> > > > specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
> > > > function prototypes in pci.h are also wrapped with "#ifdef
> > > > CONFIG_DM_PCI". The former option will build and link DM PCI code which
> > > > is not used and therefore bloats the U-Boot binary.
> > > > 
> > > > I guess the choice mainly depends on whether you'll add a PCI host
> > > > controller driver for Octeon MIPS64 in the future and can live with the
> > > > extra but unused PCI code until then.
> > > 
> > > We can definitely live with the temporarily unused PCI code. I don't
> > > want to add these #ifdefs again, which I removed explicitly upon Simon's
> > > request.
> > > 
> > > To solve this, I would prefer to add a "select DM_PCI & PCI" to
> > > arch/mips/Kconfig for Octeon, as this PCI probing construct is not
> > > only used in this GPIO driver, but also in many other drivers shared
> > > between MIPS Octeon and the also upcoming ARM64 Octeon TX/TX2 support,
> > > like I2C, SPI etc. Here all devices are PCI based hence the PCI probing
> > > dependency.
> > > 
> > > Is this okay with you? Or should I stay with the dependency in the
> > > drivers Kconfig file?
> > > 
> > 
> > I would also prefer a "select DM_PCI". I just struggle a bit with the
> > "& PCI" part. IMHO DM_PCI should not depend on PCI because DM_PCI
> > should be a hidden option which is selected by the SoC dependent on the
> > SoC specific drivers and whether them support DM. It doesn't make sense
> > to let the user choose all those DM_* options. Most other DM_* options
> > already work like this except that them are not hidden.
> > 
> > BTW: When you prepare a patch, you can also "select DM_I2C" and
> > "DM_SPI" which have the same issues with the PCI dependency.
> 
> Sure. But selecting only "DM_PCI" and not "PCI" leads to these issues:
> 
> WARNING: unmet direct dependencies detected for DM_PCI
>    Depends on [n]: PCI [=n] && DM [=y]
>    Selected by [y]:
>    - ARCH_OCTEON [=y] && <choice>
> 
> It doesn't make sense to have DM_PCI enabled and PCI not. Same happens
> with DM_SPI BTW.

exactly that is my problem. The dependency doesn't make sense and
should be removed IMHO. Options like DM_PCI should be interpreted as
"this platform with its drivers support PCI witrh DM". It doesn't nake
sense to let an user choose this option. I just wanted to point out
this inconsistency. Maybe Simon has an opinion about that.

> 
> How would you like me to solve this? Enable PCI (and SPI etc) in the
> board defconfig file? This will lead to problems with git-bisecting
> though, as the Kconfig change depends on the defconfig change and
> vice versa. I'll squash the Kconfig and defconfig updates in one
> patch because of this.

with the current state it's either "select DM_PCI & PCI" as you
suggested or adding CONFIG_DM_PCI=y and CONFIG_PCI=y to your defconfig.
The choice mainly depends on whether a subsystem driver like GPIO using
DM_PCI is optional for the user or is always required. 

Anyway all Octeon drivers using DM_PCI API needs a "depends on DM_PCI"
so that you can't enable those drivers without enabling DM_PCI.
Otherwise an user will get build errors.

> 
> I'll combine all these changes including the GPIO driver update, all
> the dts updates and a newly introduced simple clk driver in a new
> patchset shortly.
> 
> Thanks,
> Stefan
-- 
- Daniel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-07-22 11:47         ` Daniel Schwierzeck
@ 2020-07-23 10:07           ` Stefan Roese
  2020-07-28 18:58           ` Simon Glass
  1 sibling, 0 replies; 11+ messages in thread
From: Stefan Roese @ 2020-07-23 10:07 UTC (permalink / raw)
  To: u-boot

On 22.07.20 13:47, Daniel Schwierzeck wrote:
>> Hi Daniel,
>>
>> On 21.07.20 16:59, Daniel Schwierzeck wrote:
>>>> Hi Daniel,
>>>>
>>>> On 18.07.20 15:25, Daniel Schwierzeck wrote:
>>>>>> From: Suneel Garapati <sgarapati@marvell.com>
>>> ...
>>>>> found another issue:
>>>>>
>>>>> drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
>>>>> drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
>>>>> function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
>>>>> function-declaration]
>>>>>      189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
>>>>>          |                ^~~~~~~~~~~~~~
>>>>>          |                pci_map_bar
>>>>> drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
>>>>> 'int' makes pointer from integer without a cast [-Wint-conversion]
>>>>>      189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
>>>>>          |              ^
>>>>
>>>> Ah, I did not see this in my local builds, as I have enabled DM_PCI
>>>> here and forgot to add this dependency.
>>>>
>>>>> due to the dependency on DM_PCI you need to express this in Kconfig
>>>>> with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
>>>>> specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
>>>>> function prototypes in pci.h are also wrapped with "#ifdef
>>>>> CONFIG_DM_PCI". The former option will build and link DM PCI code which
>>>>> is not used and therefore bloats the U-Boot binary.
>>>>>
>>>>> I guess the choice mainly depends on whether you'll add a PCI host
>>>>> controller driver for Octeon MIPS64 in the future and can live with the
>>>>> extra but unused PCI code until then.
>>>>
>>>> We can definitely live with the temporarily unused PCI code. I don't
>>>> want to add these #ifdefs again, which I removed explicitly upon Simon's
>>>> request.
>>>>
>>>> To solve this, I would prefer to add a "select DM_PCI & PCI" to
>>>> arch/mips/Kconfig for Octeon, as this PCI probing construct is not
>>>> only used in this GPIO driver, but also in many other drivers shared
>>>> between MIPS Octeon and the also upcoming ARM64 Octeon TX/TX2 support,
>>>> like I2C, SPI etc. Here all devices are PCI based hence the PCI probing
>>>> dependency.
>>>>
>>>> Is this okay with you? Or should I stay with the dependency in the
>>>> drivers Kconfig file?
>>>>
>>>
>>> I would also prefer a "select DM_PCI". I just struggle a bit with the
>>> "& PCI" part. IMHO DM_PCI should not depend on PCI because DM_PCI
>>> should be a hidden option which is selected by the SoC dependent on the
>>> SoC specific drivers and whether them support DM. It doesn't make sense
>>> to let the user choose all those DM_* options. Most other DM_* options
>>> already work like this except that them are not hidden.
>>>
>>> BTW: When you prepare a patch, you can also "select DM_I2C" and
>>> "DM_SPI" which have the same issues with the PCI dependency.
>>
>> Sure. But selecting only "DM_PCI" and not "PCI" leads to these issues:
>>
>> WARNING: unmet direct dependencies detected for DM_PCI
>>     Depends on [n]: PCI [=n] && DM [=y]
>>     Selected by [y]:
>>     - ARCH_OCTEON [=y] && <choice>
>>
>> It doesn't make sense to have DM_PCI enabled and PCI not. Same happens
>> with DM_SPI BTW.
> 
> exactly that is my problem. The dependency doesn't make sense and
> should be removed IMHO. Options like DM_PCI should be interpreted as
> "this platform with its drivers support PCI witrh DM". It doesn't nake
> sense to let an user choose this option. I just wanted to point out
> this inconsistency. Maybe Simon has an opinion about that.
> 
>>
>> How would you like me to solve this? Enable PCI (and SPI etc) in the
>> board defconfig file? This will lead to problems with git-bisecting
>> though, as the Kconfig change depends on the defconfig change and
>> vice versa. I'll squash the Kconfig and defconfig updates in one
>> patch because of this.
> 
> with the current state it's either "select DM_PCI & PCI" as you
> suggested or adding CONFIG_DM_PCI=y and CONFIG_PCI=y to your defconfig.
> The choice mainly depends on whether a subsystem driver like GPIO using
> DM_PCI is optional for the user or is always required.

Since most subsystem drivers like GPIO etc are optional, I would like to
move to enabling PCI & DM_PCI via defconfig. I'll remove the selection
of DM_PCI from arch/mips/Kconfig then to make it more consistant.

> Anyway all Octeon drivers using DM_PCI API needs a "depends on DM_PCI"
> so that you can't enable those drivers without enabling DM_PCI.
> Otherwise an user will get build errors.

I've updated the Kconfig file(s) with this dependency in the next
patchset version.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
  2020-07-22 11:47         ` Daniel Schwierzeck
  2020-07-23 10:07           ` Stefan Roese
@ 2020-07-28 18:58           ` Simon Glass
  1 sibling, 0 replies; 11+ messages in thread
From: Simon Glass @ 2020-07-28 18:58 UTC (permalink / raw)
  To: u-boot

Hi Daniel,

On Wed, 22 Jul 2020 at 05:47, Daniel Schwierzeck
<daniel.schwierzeck@gmail.com> wrote:
>
> > Hi Daniel,
> >
> > On 21.07.20 16:59, Daniel Schwierzeck wrote:
> > > > Hi Daniel,
> > > >
> > > > On 18.07.20 15:25, Daniel Schwierzeck wrote:
> > > > > > From: Suneel Garapati <sgarapati@marvell.com>
> > > ...
> > > > > found another issue:
> > > > >
> > > > > drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
> > > > > drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
> > > > > function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
> > > > > function-declaration]
> > > > >     189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> > > > >         |                ^~~~~~~~~~~~~~
> > > > >         |                pci_map_bar
> > > > > drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
> > > > > 'int' makes pointer from integer without a cast [-Wint-conversion]
> > > > >     189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> > > > >         |              ^
> > > >
> > > > Ah, I did not see this in my local builds, as I have enabled DM_PCI
> > > > here and forgot to add this dependency.
> > > >
> > > > > due to the dependency on DM_PCI you need to express this in Kconfig
> > > > > with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
> > > > > specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
> > > > > function prototypes in pci.h are also wrapped with "#ifdef
> > > > > CONFIG_DM_PCI". The former option will build and link DM PCI code which
> > > > > is not used and therefore bloats the U-Boot binary.
> > > > >
> > > > > I guess the choice mainly depends on whether you'll add a PCI host
> > > > > controller driver for Octeon MIPS64 in the future and can live with the
> > > > > extra but unused PCI code until then.
> > > >
> > > > We can definitely live with the temporarily unused PCI code. I don't
> > > > want to add these #ifdefs again, which I removed explicitly upon Simon's
> > > > request.
> > > >
> > > > To solve this, I would prefer to add a "select DM_PCI & PCI" to
> > > > arch/mips/Kconfig for Octeon, as this PCI probing construct is not
> > > > only used in this GPIO driver, but also in many other drivers shared
> > > > between MIPS Octeon and the also upcoming ARM64 Octeon TX/TX2 support,
> > > > like I2C, SPI etc. Here all devices are PCI based hence the PCI probing
> > > > dependency.
> > > >
> > > > Is this okay with you? Or should I stay with the dependency in the
> > > > drivers Kconfig file?
> > > >
> > >
> > > I would also prefer a "select DM_PCI". I just struggle a bit with the
> > > "& PCI" part. IMHO DM_PCI should not depend on PCI because DM_PCI
> > > should be a hidden option which is selected by the SoC dependent on the
> > > SoC specific drivers and whether them support DM. It doesn't make sense
> > > to let the user choose all those DM_* options. Most other DM_* options
> > > already work like this except that them are not hidden.
> > >
> > > BTW: When you prepare a patch, you can also "select DM_I2C" and
> > > "DM_SPI" which have the same issues with the PCI dependency.
> >
> > Sure. But selecting only "DM_PCI" and not "PCI" leads to these issues:
> >
> > WARNING: unmet direct dependencies detected for DM_PCI
> >    Depends on [n]: PCI [=n] && DM [=y]
> >    Selected by [y]:
> >    - ARCH_OCTEON [=y] && <choice>
> >
> > It doesn't make sense to have DM_PCI enabled and PCI not. Same happens
> > with DM_SPI BTW.
>
> exactly that is my problem. The dependency doesn't make sense and
> should be removed IMHO. Options like DM_PCI should be interpreted as
> "this platform with its drivers support PCI witrh DM". It doesn't nake
> sense to let an user choose this option. I just wanted to point out
> this inconsistency. Maybe Simon has an opinion about that.

Yes that's right. We have it as an option since supporting DM_PCI can
be board-by-board at present. Once migration is finished, we can drop
DM_PCI.

[..]

Regards,
Simon

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-07-28 18:58 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-26 12:19 [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon Stefan Roese
2020-05-31 14:08 ` Simon Glass
2020-07-16 18:44 ` Daniel Schwierzeck
2020-07-17  6:03   ` Stefan Roese
2020-07-18 13:25 ` Daniel Schwierzeck
2020-07-21 11:03   ` Stefan Roese
2020-07-21 14:59     ` Daniel Schwierzeck
2020-07-22  8:54       ` Stefan Roese
2020-07-22 11:47         ` Daniel Schwierzeck
2020-07-23 10:07           ` Stefan Roese
2020-07-28 18:58           ` Simon Glass

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.