Re: [PATCH v2 1/2] gpio/aspeed-sgpio: enable access to all 80 input & output sgpios

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi GPIO maintainers,

On Fri, 11 Sep 2020 at 02:20, Joel Stanley <joel@xxxxxxxxx> wrote:
>
> On Fri, 11 Sep 2020 at 02:11, Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Currently, the aspeed-sgpio driver exposes up to 80 GPIO lines,
> > corresponding to the 80 status bits available in hardware. Each of these
> > lines can be configured as either an input or an output.
> >
> > However, each of these GPIOs is actually an input *and* an output; we
> > actually have 80 inputs plus 80 outputs.
> >
> > This change expands the maximum number of GPIOs to 160; the lower half
> > of this range are the input-only GPIOs, the upper half are the outputs.
> > We fix the GPIO directions to correspond to this mapping.
> >
> > This also fixes a bug when setting GPIOs - we were reading from the
> > input register, making it impossible to set more than one output GPIO.
> >
> > Signed-off-by: Jeremy Kerr <jk@xxxxxxxxxxxxxxxxxxxx>
> > Fixes: 7db47faae79b ("gpio: aspeed: Add SGPIO driver")
>
> Reviewed-by: Joel Stanley <joel@xxxxxxxxx>

This series is good to go in for 5.10:

 https://lore.kernel.org/linux-gpio/20200911015105.48581-1-jk@xxxxxxxxxxxxxxxxxxxx/
 https://lore.kernel.org/linux-gpio/20200911015105.48581-2-jk@xxxxxxxxxxxxxxxxxxxx/

Cheers,

Joel

>
> >
> > ---
> > v2:
> >  - Fix warnings from kbuild test robot
> >  - Add comment for input/output GPIO numbering
> > ---
> >  .../devicetree/bindings/gpio/sgpio-aspeed.txt |   5 +-
> >  drivers/gpio/gpio-aspeed-sgpio.c              | 126 ++++++++++++------
> >  2 files changed, 87 insertions(+), 44 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > index d4d83916c09d..be329ea4794f 100644
> > --- a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > @@ -20,8 +20,9 @@ Required properties:
> >  - gpio-controller : Marks the device node as a GPIO controller
> >  - interrupts : Interrupt specifier, see interrupt-controller/interrupts.txt
> >  - interrupt-controller : Mark the GPIO controller as an interrupt-controller
> > -- ngpios : number of GPIO lines, see gpio.txt
> > -  (should be multiple of 8, up to 80 pins)
> > +- ngpios : number of *hardware* GPIO lines, see gpio.txt. This will expose
> > +  2 software GPIOs per hardware GPIO: one for hardware input, one for hardware
> > +  output. Up to 80 pins, must be a multiple of 8.
> >  - clocks : A phandle to the APB clock for SGPM clock division
> >  - bus-frequency : SGPM CLK frequency
> >
> > diff --git a/drivers/gpio/gpio-aspeed-sgpio.c b/drivers/gpio/gpio-aspeed-sgpio.c
> > index 3aa45934d60c..a18ca52432e0 100644
> > --- a/drivers/gpio/gpio-aspeed-sgpio.c
> > +++ b/drivers/gpio/gpio-aspeed-sgpio.c
> > @@ -17,7 +17,17 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/string.h>
> >
> > -#define MAX_NR_SGPIO                   80
> > +/*
> > + * MAX_NR_HW_GPIO represents the number of actual hardware-supported GPIOs (ie,
> > + * slots within the clocked serial GPIO data). Since each HW GPIO is both an
> > + * input and an output, we provide MAX_NR_HW_GPIO * 2 lines on our gpiochip
> > + * device.
> > + *
> > + * We use SGPIO_OUTPUT_OFFSET to define the split between the inputs and
> > + * outputs; the inputs start at line 0, the outputs start at OUTPUT_OFFSET.
> > + */
> > +#define MAX_NR_HW_SGPIO                        80
> > +#define SGPIO_OUTPUT_OFFSET            MAX_NR_HW_SGPIO
> >
> >  #define ASPEED_SGPIO_CTRL              0x54
> >
> > @@ -30,8 +40,8 @@ struct aspeed_sgpio {
> >         struct clk *pclk;
> >         spinlock_t lock;
> >         void __iomem *base;
> > -       uint32_t dir_in[3];
> >         int irq;
> > +       int n_sgpio;
> >  };
> >
> >  struct aspeed_sgpio_bank {
> > @@ -111,31 +121,69 @@ static void __iomem *bank_reg(struct aspeed_sgpio *gpio,
> >         }
> >  }
> >
> > -#define GPIO_BANK(x)    ((x) >> 5)
> > -#define GPIO_OFFSET(x)  ((x) & 0x1f)
> > +#define GPIO_BANK(x)    ((x % SGPIO_OUTPUT_OFFSET) >> 5)
> > +#define GPIO_OFFSET(x)  ((x % SGPIO_OUTPUT_OFFSET) & 0x1f)
> >  #define GPIO_BIT(x)     BIT(GPIO_OFFSET(x))
> >
> >  static const struct aspeed_sgpio_bank *to_bank(unsigned int offset)
> >  {
> > -       unsigned int bank = GPIO_BANK(offset);
> > +       unsigned int bank;
> > +
> > +       bank = GPIO_BANK(offset);
> >
> >         WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> >         return &aspeed_sgpio_banks[bank];
> >  }
> >
> > +static int aspeed_sgpio_init_valid_mask(struct gpio_chip *gc,
> > +               unsigned long *valid_mask, unsigned int ngpios)
> > +{
> > +       struct aspeed_sgpio *sgpio = gpiochip_get_data(gc);
> > +       int n = sgpio->n_sgpio;
> > +       int c = SGPIO_OUTPUT_OFFSET - n;
> > +
> > +       WARN_ON(ngpios < MAX_NR_HW_SGPIO * 2);
> > +
> > +       /* input GPIOs in the lower range */
> > +       bitmap_set(valid_mask, 0, n);
> > +       bitmap_clear(valid_mask, n, c);
> > +
> > +       /* output GPIOS above SGPIO_OUTPUT_OFFSET */
> > +       bitmap_set(valid_mask, SGPIO_OUTPUT_OFFSET, n);
> > +       bitmap_clear(valid_mask, SGPIO_OUTPUT_OFFSET + n, c);
> > +
> > +       return 0;
> > +}
> > +
> > +static void aspeed_sgpio_irq_init_valid_mask(struct gpio_chip *gc,
> > +               unsigned long *valid_mask, unsigned int ngpios)
> > +{
> > +       struct aspeed_sgpio *sgpio = gpiochip_get_data(gc);
> > +       int n = sgpio->n_sgpio;
> > +
> > +       WARN_ON(ngpios < MAX_NR_HW_SGPIO * 2);
> > +
> > +       /* input GPIOs in the lower range */
> > +       bitmap_set(valid_mask, 0, n);
> > +       bitmap_clear(valid_mask, n, ngpios - n);
> > +}
> > +
> > +static bool aspeed_sgpio_is_input(unsigned int offset)
> > +{
> > +       return offset < SGPIO_OUTPUT_OFFSET;
> > +}
> > +
> >  static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
> >  {
> >         struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> >         const struct aspeed_sgpio_bank *bank = to_bank(offset);
> >         unsigned long flags;
> >         enum aspeed_sgpio_reg reg;
> > -       bool is_input;
> >         int rc = 0;
> >
> >         spin_lock_irqsave(&gpio->lock, flags);
> >
> > -       is_input = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
> > -       reg = is_input ? reg_val : reg_rdata;
> > +       reg = aspeed_sgpio_is_input(offset) ? reg_val : reg_rdata;
> >         rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
> >
> >         spin_unlock_irqrestore(&gpio->lock, flags);
> > @@ -143,22 +191,31 @@ static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
> >         return rc;
> >  }
> >
> > -static void sgpio_set_value(struct gpio_chip *gc, unsigned int offset, int val)
> > +static int sgpio_set_value(struct gpio_chip *gc, unsigned int offset, int val)
> >  {
> >         struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> >         const struct aspeed_sgpio_bank *bank = to_bank(offset);
> > -       void __iomem *addr;
> > +       void __iomem *addr_r, *addr_w;
> >         u32 reg = 0;
> >
> > -       addr = bank_reg(gpio, bank, reg_val);
> > -       reg = ioread32(addr);
> > +       if (aspeed_sgpio_is_input(offset))
> > +               return -EINVAL;
> > +
> > +       /* Since this is an output, read the cached value from rdata, then
> > +        * update val. */
> > +       addr_r = bank_reg(gpio, bank, reg_rdata);
> > +       addr_w = bank_reg(gpio, bank, reg_val);
> > +
> > +       reg = ioread32(addr_r);
> >
> >         if (val)
> >                 reg |= GPIO_BIT(offset);
> >         else
> >                 reg &= ~GPIO_BIT(offset);
> >
> > -       iowrite32(reg, addr);
> > +       iowrite32(reg, addr_w);
> > +
> > +       return 0;
> >  }
> >
> >  static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
> > @@ -175,43 +232,28 @@ static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
> >
> >  static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
> >  {
> > -       struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > -       unsigned long flags;
> > -
> > -       spin_lock_irqsave(&gpio->lock, flags);
> > -       gpio->dir_in[GPIO_BANK(offset)] |= GPIO_BIT(offset);
> > -       spin_unlock_irqrestore(&gpio->lock, flags);
> > -
> > -       return 0;
> > +       return aspeed_sgpio_is_input(offset) ? 0 : -EINVAL;
> >  }
> >
> >  static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
> >  {
> >         struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> >         unsigned long flags;
> > +       int rc;
> >
> > -       spin_lock_irqsave(&gpio->lock, flags);
> > -
> > -       gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
> > -       sgpio_set_value(gc, offset, val);
> > +       /* No special action is required for setting the direction; we'll
> > +        * error-out in sgpio_set_value if this isn't an output GPIO */
> >
> > +       spin_lock_irqsave(&gpio->lock, flags);
> > +       rc = sgpio_set_value(gc, offset, val);
> >         spin_unlock_irqrestore(&gpio->lock, flags);
> >
> > -       return 0;
> > +       return rc;
> >  }
> >
> >  static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> >  {
> > -       int dir_status;
> > -       struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > -       unsigned long flags;
> > -
> > -       spin_lock_irqsave(&gpio->lock, flags);
> > -       dir_status = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
> > -       spin_unlock_irqrestore(&gpio->lock, flags);
> > -
> > -       return dir_status;
> > -
> > +       return !!aspeed_sgpio_is_input(offset);
> >  }
> >
> >  static void irqd_to_aspeed_sgpio_data(struct irq_data *d,
> > @@ -402,6 +444,7 @@ static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> >
> >         irq = &gpio->chip.irq;
> >         irq->chip = &aspeed_sgpio_irqchip;
> > +       irq->init_valid_mask = aspeed_sgpio_irq_init_valid_mask;
> >         irq->handler = handle_bad_irq;
> >         irq->default_type = IRQ_TYPE_NONE;
> >         irq->parent_handler = aspeed_sgpio_irq_handler;
> > @@ -452,11 +495,12 @@ static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> >         if (rc < 0) {
> >                 dev_err(&pdev->dev, "Could not read ngpios property\n");
> >                 return -EINVAL;
> > -       } else if (nr_gpios > MAX_NR_SGPIO) {
> > +       } else if (nr_gpios > MAX_NR_HW_SGPIO) {
> >                 dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: %d\n",
> > -                       MAX_NR_SGPIO, nr_gpios);
> > +                       MAX_NR_HW_SGPIO, nr_gpios);
> >                 return -EINVAL;
> >         }
> > +       gpio->n_sgpio = nr_gpios;
> >
> >         rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
> >         if (rc < 0) {
> > @@ -497,7 +541,8 @@ static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> >         spin_lock_init(&gpio->lock);
> >
> >         gpio->chip.parent = &pdev->dev;
> > -       gpio->chip.ngpio = nr_gpios;
> > +       gpio->chip.ngpio = MAX_NR_HW_SGPIO * 2;
> > +       gpio->chip.init_valid_mask = aspeed_sgpio_init_valid_mask;
> >         gpio->chip.direction_input = aspeed_sgpio_dir_in;
> >         gpio->chip.direction_output = aspeed_sgpio_dir_out;
> >         gpio->chip.get_direction = aspeed_sgpio_get_direction;
> > @@ -509,9 +554,6 @@ static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> >         gpio->chip.label = dev_name(&pdev->dev);
> >         gpio->chip.base = -1;
> >
> > -       /* set all SGPIO pins as input (1). */
> > -       memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in));
> > -
> >         aspeed_sgpio_setup_irqs(gpio, pdev);
> >
> >         rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> > --
> > 2.28.0
> >



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux