Re: [WIP] Port xilinx gpio to GPIO_GENERIC

[Michal Simek <michal.simek@xxxxxxxxxx>]

On 11.8.2018 21:37, Hedges  Alexander wrote:
> Hi,
> 
> This is my first try switching gpio-xilinx to GPIO_GENERIC. Most of the work
> here is based on the EP93xx GPIO which is the only example I found using
> multiple banks and bgpio_init.
> 
> The way this sets the gpio up is that with respect to the device tree, both
> banks can be addressed in the same &gpio (I hope, I havent tested addressing the
> second bank yet, mainly because no leds are mapped to it yet). But in the sysfs
> (in /sys/class/gpio/), there are two entries, one for each bank.
> 
> Something that still needs to be fixed in this implementation is that the pin
> numbers are fixed. The first pin from the first bank is automatically assigned
> the number 0 and the second bank is adjacent to that. In general setting the pin
> number to a fixed value prevents also using other gpio controllers. Is there a
> way to find out which start pin (gc->base) a gpio_chip has been assigned when
> gc->base has been set to -1?
> 
> The other changes are removing all the functions that are already implemented by
> gpio-mmio.c.
> 
> Once this is implemented. My plan is to handle IRQ support in another commit and
> then cherry-pick some of the commits from the linux-xlnx codebase to ensure
> feature parity.
> 
> Regards,
> Alexander
> 
> ---
>  drivers/gpio/Kconfig       |   1 +
>  drivers/gpio/gpio-xilinx.c | 357 +++++++------------------------------
>  2 files changed, 69 insertions(+), 289 deletions(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 71c0ab46f216..5101919937b3 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -564,6 +564,7 @@ config GPIO_XGENE_SB
>  config GPIO_XILINX
>  	tristate "Xilinx GPIO support"
>  	depends on OF_GPIO
> +	select GPIO_GENERIC
>  	help
>  	  Say yes here to support the Xilinx FPGA GPIO device
> 
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index e8ec0e33a0a9..161b41d613e0 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -29,246 +29,17 @@
> 
>  #define XGPIO_CHANNEL_OFFSET	0x8
> 
> -/* Read/Write access to the GPIO registers */
> -#if defined(CONFIG_ARCH_ZYNQ) || defined(CONFIG_X86)
> -# define xgpio_readreg(offset)		readl(offset)
> -# define xgpio_writereg(offset, val)	writel(val, offset)
> -#else
> -# define xgpio_readreg(offset)		__raw_readl(offset)
> -# define xgpio_writereg(offset, val)	__raw_writel(val, offset)
> -#endif
> -
>  /**
>   * struct xgpio_instance - Stores information about GPIO device
> - * @mmchip: OF GPIO chip for memory mapped banks
> - * @gpio_width: GPIO width for every channel
> - * @gpio_state: GPIO state shadow register
> - * @gpio_dir: GPIO direction shadow register
> - * @gpio_lock: Lock used for synchronization
> + * @gc: GPIO chips for memory mapped banks
> + * @is_dual: True if the second bank is used
>   */
>  struct xgpio_instance {
> -	struct of_mm_gpio_chip mmchip;
> -	unsigned int gpio_width[2];
> -	u32 gpio_state[2];
> -	u32 gpio_dir[2];
> -	spinlock_t gpio_lock[2];
> +	struct gpio_chip gc[2];
> +	bool is_dual;
> +	void __iomem *base;
>  };
> 
> -static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
> -{
> -	if (gpio >= chip->gpio_width[0])
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static inline int xgpio_regoffset(struct xgpio_instance *chip, int gpio)
> -{
> -	if (xgpio_index(chip, gpio))
> -		return XGPIO_CHANNEL_OFFSET;
> -
> -	return 0;
> -}
> -
> -static inline int xgpio_offset(struct xgpio_instance *chip, int gpio)
> -{
> -	if (xgpio_index(chip, gpio))
> -		return gpio - chip->gpio_width[0];
> -
> -	return gpio;
> -}
> -
> -/**
> - * xgpio_get - Read the specified signal of the GPIO device.
> - * @gc:     Pointer to gpio_chip device structure.
> - * @gpio:   GPIO signal number.
> - *
> - * This function reads the specified signal of the GPIO device.
> - *
> - * Return:
> - * 0 if direction of GPIO signals is set as input otherwise it
> - * returns negative error value.
> - */
> -static int xgpio_get(struct gpio_chip *gc, unsigned int gpio)
> -{
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct xgpio_instance *chip = gpiochip_get_data(gc);
> -	u32 val;
> -
> -	val = xgpio_readreg(mm_gc->regs + XGPIO_DATA_OFFSET +
> -			    xgpio_regoffset(chip, gpio));
> -
> -	return !!(val & BIT(xgpio_offset(chip, gpio)));
> -}
> -
> -/**
> - * xgpio_set - Write the specified signal of the GPIO device.
> - * @gc:     Pointer to gpio_chip device structure.
> - * @gpio:   GPIO signal number.
> - * @val:    Value to be written to specified signal.
> - *
> - * This function writes the specified value in to the specified signal of the
> - * GPIO device.
> - */
> -static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> -{
> -	unsigned long flags;
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct xgpio_instance *chip = gpiochip_get_data(gc);
> -	int index =  xgpio_index(chip, gpio);
> -	int offset =  xgpio_offset(chip, gpio);
> -
> -	spin_lock_irqsave(&chip->gpio_lock[index], flags);
> -
> -	/* Write to GPIO signal and set its direction to output */
> -	if (val)
> -		chip->gpio_state[index] |= BIT(offset);
> -	else
> -		chip->gpio_state[index] &= ~BIT(offset);
> -
> -	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET +
> -		       xgpio_regoffset(chip, gpio), chip->gpio_state[index]);
> -
> -	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> -}
> -
> -/**
> - * xgpio_set_multiple - Write the specified signals of the GPIO device.
> - * @gc:     Pointer to gpio_chip device structure.
> - * @mask:   Mask of the GPIOS to modify.
> - * @bits:   Value to be wrote on each GPIO
> - *
> - * This function writes the specified values into the specified signals of the
> - * GPIO devices.
> - */
> -static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> -			       unsigned long *bits)
> -{
> -	unsigned long flags;
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct xgpio_instance *chip = gpiochip_get_data(gc);
> -	int index = xgpio_index(chip, 0);
> -	int offset, i;
> -
> -	spin_lock_irqsave(&chip->gpio_lock[index], flags);
> -
> -	/* Write to GPIO signals */
> -	for (i = 0; i < gc->ngpio; i++) {
> -		if (*mask == 0)
> -			break;
> -		if (index !=  xgpio_index(chip, i)) {
> -			xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET +
> -				       xgpio_regoffset(chip, i),
> -				       chip->gpio_state[index]);
> -			spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> -			index =  xgpio_index(chip, i);
> -			spin_lock_irqsave(&chip->gpio_lock[index], flags);
> -		}
> -		if (__test_and_clear_bit(i, mask)) {
> -			offset =  xgpio_offset(chip, i);
> -			if (test_bit(i, bits))
> -				chip->gpio_state[index] |= BIT(offset);
> -			else
> -				chip->gpio_state[index] &= ~BIT(offset);
> -		}
> -	}
> -
> -	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET +
> -		       xgpio_regoffset(chip, i), chip->gpio_state[index]);
> -
> -	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> -}
> -
> -/**
> - * xgpio_dir_in - Set the direction of the specified GPIO signal as input.
> - * @gc:     Pointer to gpio_chip device structure.
> - * @gpio:   GPIO signal number.
> - *
> - * Return:
> - * 0 - if direction of GPIO signals is set as input
> - * otherwise it returns negative error value.
> - */
> -static int xgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
> -{
> -	unsigned long flags;
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct xgpio_instance *chip = gpiochip_get_data(gc);
> -	int index =  xgpio_index(chip, gpio);
> -	int offset =  xgpio_offset(chip, gpio);
> -
> -	spin_lock_irqsave(&chip->gpio_lock[index], flags);
> -
> -	/* Set the GPIO bit in shadow register and set direction as input */
> -	chip->gpio_dir[index] |= BIT(offset);
> -	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET +
> -		       xgpio_regoffset(chip, gpio), chip->gpio_dir[index]);
> -
> -	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> -
> -	return 0;
> -}
> -
> -/**
> - * xgpio_dir_out - Set the direction of the specified GPIO signal as output.
> - * @gc:     Pointer to gpio_chip device structure.
> - * @gpio:   GPIO signal number.
> - * @val:    Value to be written to specified signal.
> - *
> - * This function sets the direction of specified GPIO signal as output.
> - *
> - * Return:
> - * If all GPIO signals of GPIO chip is configured as input then it returns
> - * error otherwise it returns 0.
> - */
> -static int xgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> -{
> -	unsigned long flags;
> -	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
> -	struct xgpio_instance *chip = gpiochip_get_data(gc);
> -	int index =  xgpio_index(chip, gpio);
> -	int offset =  xgpio_offset(chip, gpio);
> -
> -	spin_lock_irqsave(&chip->gpio_lock[index], flags);
> -
> -	/* Write state of GPIO signal */
> -	if (val)
> -		chip->gpio_state[index] |= BIT(offset);
> -	else
> -		chip->gpio_state[index] &= ~BIT(offset);
> -	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET +
> -			xgpio_regoffset(chip, gpio), chip->gpio_state[index]);
> -
> -	/* Clear the GPIO bit in shadow register and set direction as output */
> -	chip->gpio_dir[index] &= ~BIT(offset);
> -	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET +
> -			xgpio_regoffset(chip, gpio), chip->gpio_dir[index]);
> -
> -	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> -
> -	return 0;
> -}
> -
> -/**
> - * xgpio_save_regs - Set initial values of GPIO pins
> - * @mm_gc: Pointer to memory mapped GPIO chip structure
> - */
> -static void xgpio_save_regs(struct of_mm_gpio_chip *mm_gc)
> -{
> -	struct xgpio_instance *chip =
> -		container_of(mm_gc, struct xgpio_instance, mmchip);
> -
> -	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET,	chip->gpio_state[0]);
> -	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET, chip->gpio_dir[0]);
> -
> -	if (!chip->gpio_width[1])
> -		return;
> -
> -	xgpio_writereg(mm_gc->regs + XGPIO_DATA_OFFSET + XGPIO_CHANNEL_OFFSET,
> -		       chip->gpio_state[1]);
> -	xgpio_writereg(mm_gc->regs + XGPIO_TRI_OFFSET + XGPIO_CHANNEL_OFFSET,
> -		       chip->gpio_dir[1]);
> -}
> -
>  /**
>   * xgpio_remove - Remove method for the GPIO device.
>   * @pdev: pointer to the platform device
> @@ -281,11 +52,58 @@ static int xgpio_remove(struct platform_device *pdev)
>  {
>  	struct xgpio_instance *chip = platform_get_drvdata(pdev);
> 
> -	of_mm_gpiochip_remove(&chip->mmchip);
> +	gpiochip_remove(&chip->gc[0]);
> +	if (&chip->is_dual) {
> +		gpiochip_remove(&chip->gc[1]);
> +	}
> 
>  	return 0;
>  }
> 
> +static int xgpio_add_bank(struct gpio_chip *gc, struct device *dev,
> +	void __iomem *mmio_base, int bank, u16 base)
> +{
> +	int status = 0;
> +	u32 gpio_state;
> +	u32 gpio_dir;
> +	u32 gpio_width;
> +	const char *dout_name[2] = {"xlnx,dout-default", "xlnx,dout-default-2"};
> +	const char *tri_name[2] = {"xlnx,tri-default", "xlnx,tri-default-2"};
> +	const char *width_name[2] = {"xlnx,gpio-width", "xlnx,gpio2-width"};
> +	const char *bank_name[2] = {"GPIO1", "GPIO2"};
> +	struct device_node *np = dev->of_node;
> +
> +	/* It only supports two banks */
> +	if (bank >> 1)
> +		return -1;
> +
> +	of_property_read_u32(np, dout_name[bank], &gpio_state);
> +
> +	if (of_property_read_u32(np, tri_name[bank], &gpio_dir))
> +		gpio_dir = 0xFFFFFFFF;
> +
> +	if (of_property_read_u32(np, width_name[bank], &gpio_width))
> +		gpio_width = 32;
> +
> +	status = bgpio_init(gc, dev, 4,
> +			    mmio_base + XGPIO_DATA_OFFSET,
> +			    NULL,
> +			    NULL,
> +			    mmio_base + XGPIO_TRI_OFFSET,
> +			    NULL,
> +			    0);
> +	if (status) {
> +		return status;
> +	}
> +	gc->label = bank_name[bank];
> +	gc->ngpio = gpio_width;
> +	gc->base = base;
> +	gc->bgpio_data = gpio_state;
> +	gc->bgpio_dir = gpio_dir;
> +
> +	return devm_gpiochip_add_data(dev, gc, NULL);
> +}
> +
>  /**
>   * xgpio_of_probe - Probe method for the GPIO device.
>   * @pdev: pointer to the platform device
> @@ -297,72 +115,33 @@ static int xgpio_remove(struct platform_device *pdev)
>  static int xgpio_probe(struct platform_device *pdev)
>  {
>  	struct xgpio_instance *chip;
> -	int status = 0;
> +	struct device *dev = &pdev->dev;
>  	struct device_node *np = pdev->dev.of_node;
> +	struct resource *res;
>  	u32 is_dual;
> 
> -	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
>  		return -ENOMEM;
> 
> -	platform_set_drvdata(pdev, chip);
> -
> -	/* Update GPIO state shadow register with default value */
> -	of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state[0]);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	chip->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(chip->base))
> +		return PTR_ERR(chip->base);
> 
> -	/* Update GPIO direction shadow register with default value */
> -	if (of_property_read_u32(np, "xlnx,tri-default", &chip->gpio_dir[0]))
> -		chip->gpio_dir[0] = 0xFFFFFFFF;
> -
> -	/*
> -	 * Check device node and parent device node for device width
> -	 * and assume default width of 32
> -	 */
> -	if (of_property_read_u32(np, "xlnx,gpio-width", &chip->gpio_width[0]))
> -		chip->gpio_width[0] = 32;
> -
> -	spin_lock_init(&chip->gpio_lock[0]);
> +	if (xgpio_add_bank(&chip->gc[0], dev, chip->base, 0, 0)) {


This base 0 is weird. Please look below.

> +		dev_err(dev, "unable to init generic GPIO bank 1\n");
> +	}
> 
>  	if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
>  		is_dual = 0;
> +	chip->is_dual = is_dual;
> 
>  	if (is_dual) {
> -		/* Update GPIO state shadow register with default value */
> -		of_property_read_u32(np, "xlnx,dout-default-2",
> -				     &chip->gpio_state[1]);
> -
> -		/* Update GPIO direction shadow register with default value */
> -		if (of_property_read_u32(np, "xlnx,tri-default-2",
> -					 &chip->gpio_dir[1]))
> -			chip->gpio_dir[1] = 0xFFFFFFFF;
> -
> -		/*
> -		 * Check device node and parent device node for device width
> -		 * and assume default width of 32
> -		 */
> -		if (of_property_read_u32(np, "xlnx,gpio2-width",
> -					 &chip->gpio_width[1]))
> -			chip->gpio_width[1] = 32;
> -
> -		spin_lock_init(&chip->gpio_lock[1]);
> -	}
> -
> -	chip->mmchip.gc.ngpio = chip->gpio_width[0] + chip->gpio_width[1];
> -	chip->mmchip.gc.parent = &pdev->dev;
> -	chip->mmchip.gc.direction_input = xgpio_dir_in;
> -	chip->mmchip.gc.direction_output = xgpio_dir_out;
> -	chip->mmchip.gc.get = xgpio_get;
> -	chip->mmchip.gc.set = xgpio_set;
> -	chip->mmchip.gc.set_multiple = xgpio_set_multiple;
> -
> -	chip->mmchip.save_regs = xgpio_save_regs;
> -
> -	/* Call the OF gpio helper to setup and register the GPIO device */
> -	status = of_mm_gpiochip_add_data(np, &chip->mmchip, chip);
> -	if (status) {
> -		pr_err("%pOF: error in probe function with status %d\n",
> -		       np, status);
> -		return status;
> +		if (xgpio_add_bank(&chip->gc[1], dev,
> +				   chip->base + XGPIO_CHANNEL_OFFSET, 1,
> +				   chip->gc[0].ngpio))

The same hardcoding here. I didn't try this but you can have multiple
instances of this driver that's why you can't target 0(or this
chip->gc[0].ngpio).

> +			dev_err(dev, "unable to init generic GPIO bank 2\n");
>  	}
> 
>  	return 0;
> --
> 2.17.1
> 

Thanks,
Michal