Re: [PATCH] gpio/sifive: fix static checker warning

[JaeJoon Jung <rgbi3307@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2988 bytes --]

Because SiFive FU540 GPIO Registers are aligned 32-bit,
I think that irq_state is good "unsigned int" than "unsigned long".

I refer to SiFive FU540-C000 Manual v1p0 (GPIO Register Table 103)
as "Only naturally aligned 32-bit memory accesses are supported"

when you use assign_bit(offset, &chip->irq_state, 1),
offset is 32-bit.

I prefer to use bit operation instead of assign_bit().

u32 bit = BIT(offset);
chip->irq_state |= bit;

If you use this code, you do not use the assign_bit() and
need not change irq_state data type.

There are many improvements in my works for drivers/gpio/gpio-sifive.c.
I hope to check my attached source file.

On Tue, 28 Jan 2020 at 22:21, Marc Zyngier <maz@kernel.org> wrote:
>
> On 2020-01-28 05:24, Yash Shah wrote:
> > Typcasting "irq_state" leads to the below static checker warning:
> > The fix is to declare "irq_state" as unsigned long instead of u32.
> >
> >       drivers/gpio/gpio-sifive.c:97 sifive_gpio_irq_enable()
> >       warn: passing casted pointer '&chip->irq_state' to
> >       'assign_bit()' 32 vs 64.
> >
> > Fixes: 96868dce644d ("gpio/sifive: Add GPIO driver for SiFive SoCs")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
> > ---
> >  drivers/gpio/gpio-sifive.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
> > index 147a1bd..c54dd08 100644
> > --- a/drivers/gpio/gpio-sifive.c
> > +++ b/drivers/gpio/gpio-sifive.c
> > @@ -35,7 +35,7 @@ struct sifive_gpio {
> >       void __iomem            *base;
> >       struct gpio_chip        gc;
> >       struct regmap           *regs;
> > -     u32                     irq_state;
> > +     unsigned long           irq_state;
> >       unsigned int            trigger[SIFIVE_GPIO_MAX];
> >       unsigned int            irq_parent[SIFIVE_GPIO_MAX];
> >  };
> > @@ -94,7 +94,7 @@ static void sifive_gpio_irq_enable(struct irq_data
> > *d)
> >       spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> >
> >       /* Enable interrupts */
> > -     assign_bit(offset, (unsigned long *)&chip->irq_state, 1);
> > +     assign_bit(offset, &chip->irq_state, 1);
> >       sifive_gpio_set_ie(chip, offset);
> >  }
> >
> > @@ -104,7 +104,7 @@ static void sifive_gpio_irq_disable(struct irq_data
> > *d)
> >       struct sifive_gpio *chip = gpiochip_get_data(gc);
> >       int offset = irqd_to_hwirq(d) % SIFIVE_GPIO_MAX;
> >
> > -     assign_bit(offset, (unsigned long *)&chip->irq_state, 0);
> > +     assign_bit(offset, &chip->irq_state, 0);
> >       sifive_gpio_set_ie(chip, offset);
> >       irq_chip_disable_parent(d);
> >  }
>
> Yup, nice one. Should have spotted it.
>
> Reviewed-by: Marc Zyngier <maz@kernel.org>
>
> Linus, do you want me to queue this via the irqchip tree (given that
> it is where the original bug came from)? Or would you rather take it?
>
>          M.
> --
> Jazz is not dead. It just smells funny...
>

[-- Attachment #2: gpio-sifive.c --]
[-- Type: text/x-csrc, Size: 13727 bytes --]

/*
 * SiFive GPIO driver
 *
 * Copyright (C) 2018 SiFive, Inc.
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 * 
 * References:                                                                     
 *   SiFive FU540-C000 manual v1p0, Chapter 17 "GPIO"
 * 
 * 2020 Editted by JaeJoon Jung <rgbi3307@gmail.com>
 *
 */
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/errno.h>
#include <linux/gpio/driver.h>
#include <linux/irqchip/chained_irq.h>
#include <linux/io.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/raid/pq.h>

#define GPIO_INPUT_VAL	        0x00
#define GPIO_INPUT_EN	        0x04
#define GPIO_OUTPUT_EN	        0x08
#define GPIO_OUTPUT_VAL	        0x0C
#define GPIO_PULLUP_EN          0x10
#define GPIO_PIN_DS             0x14
#define GPIO_RISE_IE	        0x18
#define GPIO_RISE_IP	        0x1C
#define GPIO_FALL_IE	        0x20
#define GPIO_FALL_IP	        0x24
#define GPIO_HIGH_IE	        0x28
#define GPIO_HIGH_IP	        0x2C
#define GPIO_LOW_IE	        0x30
#define GPIO_LOW_IP	        0x34
#define GPIO_OUTPUT_XOR	        0x40

#define GPIO_MAX_CNT	        32
#define GPIO_ENABLE_BITS        0x83FF

//#define GPIO_SIFIVE_DEBUG
#ifdef GPIO_SIFIVE_DEBUG
        #define gpio_sifive_debug(...)	printk("GPIO: " __VA_ARGS__)
#else
        #define gpio_sifive_debug(...)
#endif

struct sifive_gpio {
	raw_spinlock_t		lock;
	void __iomem		*base;
	struct gpio_chip	gc;
	unsigned int		irq_enable;
	unsigned int		irq_type[GPIO_MAX_CNT];
	unsigned int		irq_parent[GPIO_MAX_CNT];
	struct sifive_gpio	*self_ptr[GPIO_MAX_CNT];
};


static void gpio_sifive_debug_reg(struct sifive_gpio *chip)
{
#ifdef GPIO_SIFIVE_DEBUG
        u32 value;
        int reg;

        if (!chip->base) return;
        gpio_sifive_debug("registers values ---------------------------\n");
        for (reg=GPIO_INPUT_VAL; reg<=GPIO_OUTPUT_XOR; reg+=4) {
                value = readl(chip->base + reg);
                gpio_sifive_debug("reg=[%02X], value=[%08X]\n", reg, value);
        }
        gpio_sifive_debug("irq_enable=[%08X]\n", chip->irq_enable);
        gpio_sifive_debug("irq_type=%d\n", chip->irq_type[0]);
        gpio_sifive_debug("irq_parent=%d\n", chip->irq_parent[0]);
        gpio_sifive_debug("\n");
#endif
}

static void gpio_sifive_assign_bit(void __iomem *ptr, int offset, int value)
{
	// It's frustrating that we are not allowed to use the device atomics
	// which are GUARANTEED to be supported by this device on RISC-V
	u32 bit = BIT(offset);
        u32 old = readl(ptr);

        bit = (value) ? old | bit : old & ~bit;
        writel(bit, ptr);
}

static int gpio_sifive_direction_input(struct gpio_chip *gc, unsigned offset)
{
	struct sifive_gpio *chip = gpiochip_get_data(gc);
	unsigned long flags;

	if (offset >= gc->ngpio)
		return -EINVAL;

	raw_spin_lock_irqsave(&chip->lock, flags);
	gpio_sifive_assign_bit(chip->base + GPIO_OUTPUT_EN, offset, 0);
	gpio_sifive_assign_bit(chip->base + GPIO_INPUT_EN,  offset, 1);
	raw_spin_unlock_irqrestore(&chip->lock, flags);

	return 0;
}

static int gpio_sifive_direction_output(struct gpio_chip *gc, unsigned offset
                                                            , int value)
{
	struct sifive_gpio *chip = gpiochip_get_data(gc);
	unsigned long flags;

	if (offset >= gc->ngpio)
		return -EINVAL;

	raw_spin_lock_irqsave(&chip->lock, flags);
	gpio_sifive_assign_bit(chip->base + GPIO_INPUT_EN,   offset, 0);
	gpio_sifive_assign_bit(chip->base + GPIO_OUTPUT_EN,  offset, 1);
	gpio_sifive_assign_bit(chip->base + GPIO_OUTPUT_VAL, offset, value);
	raw_spin_unlock_irqrestore(&chip->lock, flags);

	return 0;
}

static int gpio_sifive_get_direction(struct gpio_chip *gc, unsigned offset)
{
	struct sifive_gpio *chip = gpiochip_get_data(gc);
        int value;

	if (offset >= gc->ngpio)
		return -EINVAL;

        value = readl(chip->base + GPIO_OUTPUT_EN) & BIT(offset);
        return !value;
}

static int gpio_sifive_get_value(struct gpio_chip *gc, unsigned offset)
{
	struct sifive_gpio *chip = gpiochip_get_data(gc);
        int index, value;

	if (offset >= gc->ngpio)
		return -EINVAL;

        index = gpio_sifive_get_direction(gc, offset) ? 
                                GPIO_INPUT_VAL : GPIO_OUTPUT_VAL;
        value = readl(chip->base + index) & BIT(offset);
        return value;        
}

static void gpio_sifive_set_value(struct gpio_chip *gc, unsigned offset, int value)
{
	struct sifive_gpio *chip = gpiochip_get_data(gc);
	unsigned long flags;
        int index;

	if (offset >= gc->ngpio)
		return;

        index = gpio_sifive_get_direction(gc, offset) ? 
                                GPIO_INPUT_VAL : GPIO_OUTPUT_VAL;
	raw_spin_lock_irqsave(&chip->lock, flags);
	gpio_sifive_assign_bit(chip->base + index, offset, value);
	raw_spin_unlock_irqrestore(&chip->lock, flags);
}

static void gpio_sifive_set_default(struct sifive_gpio *chip, u32 bits)
{
        if (bits == GPIO_ENABLE_BITS) {
                //Input Enable/Disable
                writel(bits, chip->base + GPIO_INPUT_EN);
                return;
        }
        //Interrupts Enable/Disable
        writel(bits, chip->base + GPIO_RISE_IE);
        writel(bits, chip->base + GPIO_FALL_IE);
        writel(bits, chip->base + GPIO_HIGH_IE);
        writel(bits, chip->base + GPIO_LOW_IE);

        writel(bits, chip->base + GPIO_RISE_IP);
        writel(bits, chip->base + GPIO_FALL_IP);
        writel(bits, chip->base + GPIO_HIGH_IP);
        writel(bits, chip->base + GPIO_LOW_IP);

        chip->irq_enable = bits;
}

static void gpio_sifive_set_ie(struct sifive_gpio *chip, int offset)
{
	unsigned long flags;
	unsigned irq_type;

	raw_spin_lock_irqsave(&chip->lock, flags);
	irq_type = (chip->irq_enable & BIT(offset)) ? chip->irq_type[offset] : 0;
	gpio_sifive_assign_bit(chip->base + GPIO_RISE_IE, offset, irq_type & IRQ_TYPE_EDGE_RISING);
	gpio_sifive_assign_bit(chip->base + GPIO_FALL_IE, offset, irq_type & IRQ_TYPE_EDGE_FALLING);
	gpio_sifive_assign_bit(chip->base + GPIO_HIGH_IE, offset, irq_type & IRQ_TYPE_LEVEL_HIGH);
	gpio_sifive_assign_bit(chip->base + GPIO_LOW_IE,  offset, irq_type & IRQ_TYPE_LEVEL_LOW);
	raw_spin_unlock_irqrestore(&chip->lock, flags);
}

static int gpio_sifive_irq_set_type(struct irq_data *d, unsigned irq_type)
{
	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
	struct sifive_gpio *chip = gpiochip_get_data(gc);
	int offset = irqd_to_hwirq(d);

	if (offset < 0 || offset >= gc->ngpio)
		return -EINVAL;

	chip->irq_type[offset] = irq_type;
	gpio_sifive_set_ie(chip, offset);

	return 0;
}

/* chained_irq_{enter,exit} already mask the parent */
static void gpio_sifive_irq_mask(struct irq_data *d) { }
static void gpio_sifive_irq_unmask(struct irq_data *d) { }

static void gpio_sifive_irq_enable(struct irq_data *d)
{
	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
	struct sifive_gpio *chip = gpiochip_get_data(gc);
	int offset = irqd_to_hwirq(d) % GPIO_MAX_CNT; // must not fail
	u32 bit = BIT(offset);

	/* Switch to input */
	gpio_sifive_direction_input(gc, offset);

	/* Clear any sticky pending interrupts */
	writel(bit, chip->base + GPIO_RISE_IP);
	writel(bit, chip->base + GPIO_FALL_IP);
	writel(bit, chip->base + GPIO_HIGH_IP);
	writel(bit, chip->base + GPIO_LOW_IP);

	/* Enable interrupts */
        chip->irq_enable |= bit;
	gpio_sifive_set_ie(chip, offset);
}

static void gpio_sifive_irq_disable(struct irq_data *d)
{
	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
	struct sifive_gpio *chip = gpiochip_get_data(gc);
	int offset = irqd_to_hwirq(d) % GPIO_MAX_CNT; // must not fail
	u32 bit = BIT(offset);

	/* Disable interrupts */
        chip->irq_enable &= ~bit;
	gpio_sifive_set_ie(chip, offset);
}

static struct irq_chip gpio_sifive_irqchip = {
	.name		= "sifive-gpio",
	.irq_set_type	= gpio_sifive_irq_set_type,
	.irq_mask	= gpio_sifive_irq_mask,
	.irq_unmask	= gpio_sifive_irq_unmask,
	.irq_enable	= gpio_sifive_irq_enable,
	.irq_disable	= gpio_sifive_irq_disable,
};

static void gpio_sifive_irq_handler(struct irq_desc *desc)
{
	struct irq_chip *irqchip = irq_desc_get_chip(desc);
	struct sifive_gpio **self_ptr = irq_desc_get_handler_data(desc);
	struct sifive_gpio *chip = *self_ptr;
	int offset = self_ptr - &chip->self_ptr[0];
        //int offset = desc->irq_data.irq - chip->irq_parent[0];
	u32 bit = BIT(offset);

	chained_irq_enter(irqchip, desc);

	/* Re-arm the edge irq_types so don't miss the next one */
	writel(bit, chip->base + GPIO_RISE_IP);
	writel(bit, chip->base + GPIO_FALL_IP);

	generic_handle_irq(irq_find_mapping(chip->gc.irq.domain, offset));

	/* Re-arm the level irq_types after handling to prevent spurious refire */
	writel(bit, chip->base + GPIO_HIGH_IP);
	writel(bit, chip->base + GPIO_LOW_IP);

	chained_irq_exit(irqchip, desc);

        gpio_sifive_debug("irq handler: offset=%d\n", offset);
}

#ifdef GPIO_SIFIVE_DEBUG
static void gpio_sifive_set_irq_enable(struct sifive_gpio *chip, unsigned offset)                          
{
        u32 bit = BIT(offset);

        /* Switch to input */
        gpio_sifive_direction_input(&chip->gc, offset);

        /* Clear any sticky pending interrupts */
        writel(bit, chip->base + GPIO_RISE_IP);
        writel(bit, chip->base + GPIO_FALL_IP);
        writel(bit, chip->base + GPIO_HIGH_IP);
        writel(bit, chip->base + GPIO_LOW_IP);

        /* Enable interrupts */
        chip->irq_enable |= bit;
        gpio_sifive_set_ie(chip, offset);
}                                                                               
                                                                                
static void gpio_sifive_set_irq_disable(struct sifive_gpio *chip, unsigned offset)                          
{
        u32 bit = BIT(offset);
        chip->irq_enable &= ~bit;
        gpio_sifive_set_ie(chip, offset);
}
#endif

static int gpio_sifive_chip_setup(struct platform_device *pdev
                                        , struct sifive_gpio *chip, int ngpio)
{
	struct device *dev = &pdev->dev;
	int gpio, irq, ret;

	raw_spin_lock_init(&chip->lock);
	chip->gc.direction_input = gpio_sifive_direction_input;
	chip->gc.direction_output = gpio_sifive_direction_output;
	chip->gc.get_direction = gpio_sifive_get_direction;
	chip->gc.get = gpio_sifive_get_value;
	chip->gc.set = gpio_sifive_set_value;
	chip->gc.base = -1;
	chip->gc.ngpio = ngpio;
	chip->gc.label = dev_name(dev);
	chip->gc.parent = dev;
	chip->gc.owner = THIS_MODULE;

	ret = gpiochip_add_data(&chip->gc, chip);
	if (ret)
		return ret;

	/* Disable all GPIO interrupts before enabling parent interrupts */
        gpio_sifive_set_default(chip, 0);

	ret = gpiochip_irqchip_add(&chip->gc, &gpio_sifive_irqchip, 0
                                        , handle_simple_irq, IRQ_TYPE_NONE);
	if (ret) {
		dev_err(dev, "GPIO: could not add irqchip\n");
		gpiochip_remove(&chip->gc);
		return ret;
	}

	chip->gc.irq.num_parents = ngpio;
	chip->gc.irq.parents = &chip->irq_parent[0];
	chip->gc.irq.map = &chip->irq_parent[0];

	for (gpio = 0; gpio < ngpio; ++gpio) {
		irq = platform_get_irq(pdev, gpio);
		if (irq < 0) {
			dev_err(dev, "GPIO: invalid IRQ\n");
			gpiochip_remove(&chip->gc);
			return -ENODEV;
		}
		chip->self_ptr[gpio] = chip;
		chip->irq_parent[gpio] = irq;
		chip->irq_type[gpio] = IRQ_TYPE_LEVEL_HIGH;
	}
	for (gpio = 0; gpio < ngpio; ++gpio) {
		irq = chip->irq_parent[gpio];
		irq_set_chained_handler_and_data(irq, gpio_sifive_irq_handler
                                                , &chip->self_ptr[gpio]);
		irq_set_parent(irq_find_mapping(chip->gc.irq.domain, gpio), irq);
	}

        //Enable GPIO Input for default
        gpio_sifive_set_default(chip, GPIO_ENABLE_BITS);
        return 0;
}

static int gpio_sifive_probe(struct platform_device *pdev)
{
	struct device *dev = &pdev->dev;
	struct device_node *node = pdev->dev.of_node;
	struct sifive_gpio *chip;
	struct resource *res;
	int ngpio;

	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
	if (!chip) {
		dev_err(dev, "out of memory\n");
		return -ENOMEM;
	}

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	chip->base = devm_ioremap_resource(dev, res);
	if (IS_ERR(chip->base)) {
		dev_err(dev, "failed to allocate device memory\n");
		return PTR_ERR(chip->base);
	}
        gpio_sifive_debug_reg(chip);

	if(of_property_read_u32(node, "ngpios", &ngpio)) 
	        ngpio = of_irq_count(node);
	
	if (ngpio >= GPIO_MAX_CNT) {
		dev_err(dev, "too many ngpios.\n");
		return -EINVAL;
	}

        if (gpio_sifive_chip_setup(pdev, chip, ngpio) < 0) {
		dev_err(dev, "failed to gpio sifive setup.\n");
                return -EINVAL;
        }

	platform_set_drvdata(pdev, chip);
	dev_info(dev, "GPIO SiFive driver registered %d GPIOs\n", ngpio);

#ifdef GPIO_SIFIVE_DEBUG
        gpio_sifive_set_irq_enable(chip, 7);    ///GPIO7 interrupt enabled for test                          
        gpio_sifive_set_irq_disable(chip, 9);   ///GPIO9 interrupt disabled for test                     
#endif
        gpio_sifive_debug_reg(chip);
	return 0;
}

static const struct of_device_id gpio_sifive_match[] = {
	{
		.compatible = "sifive,gpio0",
	},
	{ },
};
MODULE_DEVICE_TABLE(of, gpio_sifive_match);

static struct platform_driver gpio_sifive_driver = {
	.probe		= gpio_sifive_probe,
	.driver = {
		.name	= "gpio-sifive",
		.of_match_table = of_match_ptr(gpio_sifive_match),
	},
};
module_platform_driver(gpio_sifive_driver);

MODULE_DESCRIPTION("SiFive GPIO driver");
MODULE_LICENSE("GPL v2");