* [v5 0/2] gpio: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-19 19:24 UTC (permalink / raw)
To: linux-aspeed
Hello,
This short series introduce dt-binding document and a driver for the
Aspeed AST2500 SGPIO controller. Please review.
The related SGPM pinmux dt-binding document, dts, and pinctrl driver
updates have been accepted and merged:
_http://patchwork.ozlabs.org/patch/1110210/
Hongwei Zhang (2):
dt-bindings: gpio: aspeed: Add SGPIO support
gpio: aspeed: Add SGPIO driver
.../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 +++
drivers/gpio/sgpio-aspeed.c | 522 +++++++++++++++++++++
2 files changed, 577 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
create mode 100644 drivers/gpio/sgpio-aspeed.c
--
2.7.4
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Andrew Jeffery @ 2019-07-18 1:49 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAL_JsqKYL5yv8E=aKD1fJwXx1LLdUAs_ZFrc5k1dHsB9u+2ing@mail.gmail.com>
On Wed, 17 Jul 2019, at 23:13, Rob Herring wrote:
> On Tue, Jul 16, 2019 at 9:58 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote:
> > > On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > >
> > > >
> > > > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> > > > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > > >
> > > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > > > > data bus if only a single slot is enabled.
> > > > > >
> > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > > > ---
> > > > > > In v2:
> > > > > >
> > > > > > * Rename to aspeed,sdhci.yaml
> > > > > > * Rename sd-controller compatible
> > > > > > * Add `maxItems: 1` for reg properties
> > > > > > * Move sdhci subnode description to patternProperties
> > > > > > * Drop sdhci compatible requirement
> > > > > > * #address-cells and #size-cells are required
> > > > > > * Prevent additional properties
> > > > > > * Implement explicit ranges in example
> > > > > > * Remove slot property
> > > > > >
> > > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > > > > 1 file changed, 90 insertions(+)
> > > > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..67a691c3348c
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > > @@ -0,0 +1,90 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: ASPEED SD/SDIO/eMMC Controller
> > > > > > +
> > > > > > +maintainers:
> > > > > > + - Andrew Jeffery <andrew@aj.id.au>
> > > > > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > > > > +
> > > > > > +description: |+
> > > > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > > > > + only a single slot is enabled.
> > > > > > +
> > > > > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > > > > + the slots are dependent on the common configuration area, they are described
> > > > > > + as child nodes.
> > > > > > +
> > > > > > +properties:
> > > > > > + compatible:
> > > > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> > > > >
> > > > > This is actually a list of 4 strings. Please reformat to 1 per line.
> > > >
> > > > On reflection that's obvious, but also a somewhat subtle interaction with the
> > > > preference for no quotes (the obvious caveat being "except where required").
> > >
> > > It wasn't something I'd run into before. I'm working on a check, but
> > > unfortunately we can only check for quotes not needed and can't check
> > > for missing quotes.
> > >
> > > > Thanks for pointing it out.
> > > >
> > > > I have been running `make dt_binding_check` and `make dtbs_check` over
> > > > these, looks like I need to up my game a bit though. Do you do additional things
> > > > in your workflow?
> > >
> > > That should have thrown the warnings. If you aren't seeing those, do
> > > you have dtschema package installed (see
> > > Documentation/devicetree/writing-schema.md)?
> >
> > I do have it installed, but as mentioned previously there's a fair few
> > warnings emitted currently by the Aspeed devicetrees, so it might have
> > got lost in the noise. I've started to clean that up, though probably need
> > some direction there too.
> >
> > Separately I'm currently trying to track down an issue where I get errors
> > on the Aspeed dts cpu nodes about failing to match the riscv CPU
> > compatibles, it seems dt-validate isn't finding the ARM CPU compatible
> > strings. It feels more annoying to track down that I'd like.
>
> There's a fix in today's linux-next for that and it should be in
> Linus' tree in a few days.
>
Thanks, I'll take a look.
Andrew
^ permalink raw reply
* [PATCH 2/3 v4] dt-bindings: gpio: aspeed: Add SGPIO support
From: Andrew Jeffery @ 2019-07-18 1:48 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563394325-15941-1-git-send-email-hongweiz@ami.com>
The subject is largely correct, but please see the discussion on the driver patch
about how to clean up the [PATCH ...] prefix.
On Thu, 18 Jul 2019, at 05:42, Hongwei Zhang wrote:
> Add bindings to support SGPIO on AST2400 or AST2500.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> new file mode 100644
> index 0000000..2d6305e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> @@ -0,0 +1,55 @@
> +Aspeed SGPIO controller Device Tree Bindings
> +-------------------------------------------
> +
> +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80
> full
> +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed
> to
> +support the following options:
> +- Support interrupt option for each input port and various interrupt
> + sensitivity option (level-high, level-low, edge-high, edge-low)
> +- Support reset tolerance option for each output port
> +- Directly connected to APB bus and its shift clock is from APB bus
> clock
> + divided by a programmable value.
> +- Co-work with external signal-chained TTL components (74LV165/74LV595)
> +
> +
> +Required properties:
> +
> +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> +
> +- #gpio-cells : Should be two
> + - First cell is the GPIO line number
> + - Second cell is used to specify optional
> + parameters (unused)
> +
> +- reg : Address and length of the register set for the device
> +- gpio-controller : Marks the device node as a GPIO controller
> +- interrupts : Interrupt specifier (see interrupt bindings for
> + details)
> +
> +- interrupt-controller : Mark the GPIO controller as an
> interrupt-controller
> +
> +- nr-gpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins)
Please change the property name to "ngpios", as per the generic GPIO
bindings[1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio.txt?h=v5.2#n141
Cheers,
Andrew
> +
> +- clocks : A phandle to the APB clock for SGPM clock
> division
> +
> +- bus-frequency : SGPM CLK frequency
> +
> +
> +The sgpio and interrupt properties are further described in their
> respective bindings documentation:
> +
> +- Documentation/devicetree/bindings/sgpio/gpio.txt
> +- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> + Example:
> + sgpio: sgpio at 1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + nr-gpios = <8>;
> + bus-frequency = <12000000>;
> + };
> --
> 2.7.4
>
>
^ permalink raw reply
* [PATCH 2/3 v4] ARM: dts: aspeed: Add SGPIO driver
From: Andrew Jeffery @ 2019-07-18 1:44 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563399020-16905-1-git-send-email-hongweiz@ami.com>
Hello Hongwei,
The driver is shaping up! I have a few remaining nitpicks below:
The first is the patch subject:
[PATCH 2/3 v4] ARM: dts: aspeed: Add SGPIO driver
I think one of the first iterations of the patch included the devicetree changes. It doesn't
any more, as the devicetree updates should be done in a separate patch, however the
subject is now inaccurate. Disregarding the [PATCH ...] prefix, it should be:
gpio: aspeed: Add SPGIO driver
As for the [PATCH ...] prefix business, both your bindings patch and your driver patch are
listed as patch 2/3, and there's no third patch in the series. `git format-patch` takes
care of all this for you - I encourage you to get `git send-email` working so you can
use it to send the patches produced from `git format-patch` (as they won't have these
problems). Further, if you're sending multiple patches it's useful to include a cover
letter as the patches will be properly threaded underneath it, which helps keep related
patches together and provides a place to talk about your patches that isn't the commit
message.
On Thu, 18 Jul 2019, at 07:00, Hongwei Zhang wrote:
> Add SGPIO driver support for Aspeed AST2500 SoC.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> drivers/gpio/sgpio-aspeed.c | 518 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 518 insertions(+)
> create mode 100644 drivers/gpio/sgpio-aspeed.c
>
> diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> new file mode 100644
> index 0000000..715052c
> --- /dev/null
> +++ b/drivers/gpio/sgpio-aspeed.c
> @@ -0,0 +1,518 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2019 American Megatrends International LLC.
> + *
> + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
> + */
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/aspeed.h>
> +#include <linux/hashtable.h>
> +#include <linux/bitfield.h>
> +#include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/gpio.h>
It would be nice if these were sorted alphabetically. Not overly fussed though.
> +
> +#define MAX_NR_SGPIO 80
> +
> +#define ASPEED_SGPIO_CTRL 0x54
> +
> +#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
> +#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
> +#define ASPEED_SGPIO_ENABLE BIT(0)
> +
> +struct aspeed_sgpio {
> + struct gpio_chip chip;
> + struct clk *pclk;
> + spinlock_t lock;
> + void __iomem *base;
> + uint32_t dir_in[3];
> + int irq;
> +};
> +
> +struct aspeed_sgpio_bank {
> + uint16_t val_regs;
> + uint16_t rdata_reg;
> + uint16_t irq_regs;
> + const char names[4][3];
> +};
> +
> +/*
> + * Note: The "value" register returns the input value when the GPIO is
> + * configured as an input.
> + *
> + * The "rdata" register returns the output value when the GPIO is
> + * configured as an output.
> + */
> +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> + {
> + .val_regs = 0x0000,
> + .rdata_reg = 0x0070,
> + .irq_regs = 0x0004,
> + .names = { "A", "B", "C", "D" },
> + },
> + {
> + .val_regs = 0x001C,
> + .rdata_reg = 0x0074,
> + .irq_regs = 0x0020,
> + .names = { "E", "F", "G", "H" },
> + },
> + {
> + .val_regs = 0x0038,
> + .rdata_reg = 0x0078,
> + .irq_regs = 0x003C,
> + .names = { "I", "J" },
> + },
> +};
> +
> +enum aspeed_sgpio_reg {
> + reg_val,
> + reg_rdata,
> + reg_irq_enable,
> + reg_irq_type0,
> + reg_irq_type1,
> + reg_irq_type2,
> + reg_irq_status,
> +};
> +
> +#define GPIO_VAL_VALUE 0x00
> +#define GPIO_IRQ_ENABLE 0x00
> +#define GPIO_IRQ_TYPE0 0x04
> +#define GPIO_IRQ_TYPE1 0x08
> +#define GPIO_IRQ_TYPE2 0x0C
> +#define GPIO_IRQ_STATUS 0x10
> +
> +/* This will be resolved at compile time */
> +static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
> + const struct aspeed_sgpio_bank *bank,
> + const enum aspeed_sgpio_reg reg)
> +{
> + switch (reg) {
> + case reg_val:
> + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> + case reg_rdata:
> + return gpio->base + bank->rdata_reg;
> + case reg_irq_enable:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> + case reg_irq_type0:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> + case reg_irq_type1:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> + case reg_irq_type2:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> + case reg_irq_status:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> + default:
> + /* acturally if code runs to here, it's an error case */
> + BUG_ON(1);
> + }
> +}
> +
> +#define GPIO_BANK(x) ((x) >> 5)
> +#define GPIO_OFFSET(x) ((x) & 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);
> +
> + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> + return &aspeed_sgpio_banks[bank];
> +}
> +
> +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;
> + rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +
> + return rc;
> +}
> +
> +static void aspeed_sgpio_set(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);
> + unsigned long flags;
> + void __iomem *addr;
> + u32 reg = 0;
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + addr = bank_reg(gpio, bank, reg_val);
> +
> + if (val)
> + reg |= GPIO_BIT(offset);
> + else
> + reg &= ~GPIO_BIT(offset);
> +
> + iowrite32(reg, addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +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;
> +}
> +
> +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;
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> + gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +
> + return 0;
> +}
> +
> +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;
> +
> +}
> +
> +static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d,
> + struct aspeed_sgpio **gpio,
> + const struct aspeed_sgpio_bank **bank,
> + u32 *bit, int *offset)
> +{
> + struct aspeed_sgpio *internal;
> +
> + *offset = irqd_to_hwirq(d);
> + internal = irq_data_get_irq_chip_data(d);
> + WARN_ON(!internal);
> +
> + *gpio = internal;
> + *bank = to_bank(*offset);
> + *bit = GPIO_BIT(*offset);
> +}
> +
> +static void aspeed_sgpio_irq_ack(struct irq_data *d)
> +{
> + const struct aspeed_sgpio_bank *bank;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + void __iomem *status_addr;
> + int offset;
> + u32 bit;
> +
> + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> +
> + status_addr = bank_reg(gpio, bank, reg_irq_status);
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + iowrite32(bit, status_addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
> +{
> + const struct aspeed_sgpio_bank *bank;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + u32 reg, bit;
> + void __iomem *addr;
> + int offset;
> +
> + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> + addr = bank_reg(gpio, bank, reg_irq_enable);
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + reg = ioread32(addr);
> + if (set)
> + reg |= bit;
> + else
> + reg &= ~bit;
> +
> + iowrite32(reg, addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static void aspeed_sgpio_irq_mask(struct irq_data *d)
> +{
> + aspeed_sgpio_irq_set_mask(d, false);
> +}
> +
> +static void aspeed_sgpio_irq_unmask(struct irq_data *d)
> +{
> + aspeed_sgpio_irq_set_mask(d, true);
> +}
> +
> +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
> +{
> + u32 type0 = 0;
> + u32 type1 = 0;
> + u32 type2 = 0;
> + u32 bit, reg;
> + const struct aspeed_sgpio_bank *bank;
> + irq_flow_handler_t handler;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + void __iomem *addr;
> + int offset;
> +
> + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> +
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_BOTH:
> + type2 |= bit;
> + /* fall through */
> + case IRQ_TYPE_EDGE_RISING:
> + type0 |= bit;
> + /* fall through */
> + case IRQ_TYPE_EDGE_FALLING:
> + handler = handle_edge_irq;
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + type0 |= bit;
> + /* fall through */
> + case IRQ_TYPE_LEVEL_LOW:
> + type1 |= bit;
> + handler = handle_level_irq;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type0);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type0;
> + iowrite32(reg, addr);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type1);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type1;
> + iowrite32(reg, addr);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type2);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type2;
> + iowrite32(reg, addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +
> + irq_set_handler_locked(d, handler);
> +
> + return 0;
> +}
> +
> +static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *ic = irq_desc_get_chip(desc);
> + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> + unsigned int i, p, girq;
> + unsigned long reg;
> +
> + chained_irq_enter(ic, desc);
> +
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> +
> + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> +
> + for_each_set_bit(p, ®, 32) {
> + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> + generic_handle_irq(girq);
> + }
> +
> + }
> +
> + chained_irq_exit(ic, desc);
> +}
> +
> +static struct irq_chip aspeed_sgpio_irqchip = {
> + .name = "aspeed-sgpio",
> + .irq_ack = aspeed_sgpio_irq_ack,
> + .irq_mask = aspeed_sgpio_irq_mask,
> + .irq_unmask = aspeed_sgpio_irq_unmask,
> + .irq_set_type = aspeed_sgpio_set_type,
> +};
> +
> +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> + struct platform_device *pdev)
> +{
> + int rc, i;
> + const struct aspeed_sgpio_bank *bank;
> +
> + rc = platform_get_irq(pdev, 0);
> + if (rc < 0)
> + return rc;
> +
> + gpio->irq = rc;
> +
> + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> Pins. */
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + bank = &aspeed_sgpio_banks[i];
> + /* disable irq enable bits */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> + /* clear status bits */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> + }
> +
> + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> + 0, handle_bad_irq, IRQ_TYPE_NONE);
> + if (rc) {
> + dev_info(&pdev->dev, "Could not add irqchip\n");
> + return rc;
> + }
> +
> + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> + gpio->irq, aspeed_sgpio_irq_handler);
> +
> + /* set IRQ settings and Enable Interrupt */
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + bank = &aspeed_sgpio_banks[i];
> + /* set falling or level-low irq */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> + /* trigger type is edge */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> + /* dual edge trigger mode. */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> + /* enable irq */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_sgpio_of_table[] = {
> + { .compatible = "aspeed,ast2400-sgpio" },
> + { .compatible = "aspeed,ast2500-sgpio" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> +
> +static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> +{
> + struct aspeed_sgpio *gpio;
> + u32 nr_gpios, sgpio_freq, sgpio_clk_div;
> + int rc;
> + unsigned long apb_freq;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + gpio->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(gpio->base))
> + return PTR_ERR(gpio->base);
> +
> + rc = of_property_read_u32(pdev->dev.of_node, "nr-gpios", &nr_gpios);
I just checked and the standard property is "ngpios", not "nr-gpios"[1]. Please fix
this and the bindings document.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio.txt?h=v5.2#n141
> + if ((rc < 0) || (nr_gpios > MAX_NR_SGPIO)) {
> + dev_err(&pdev->dev, "Could not read nr-gpios property\n");
Triggering this error message with the `nr_gpios > MAX_NR_SGPIO`
condition is a bit confusing. I think you should check that in a separate
conditional block if you would like to issue a dev_err():
if (rc < 0) {
dev_err(&pdev->dev, "Could not read nr-gpios property\n");
return -EINVAL;
} else if (nr_gpios > MAX_NR_GPIOS) {
dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: %d\n",
MAX_NR_SGPIO, nr_gpios);
return -EINVAL;
}
> + return -EINVAL;
> + }
> +
> + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency",
> &sgpio_freq);
> + if (rc < 0) {
> + dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> + return -EINVAL;
> + }
> +
> + gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(gpio->pclk)) {
> + dev_err(&pdev->dev, "devm_clk_get failed\n");
> + return PTR_ERR(gpio->pclk);
> + }
> +
> + apb_freq = clk_get_rate(gpio->pclk);
> +
> + /*
> + * From the datasheet,
> + * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
> + * period = 2 * (GPIO254[31:16] + 1) / PCLK
> + * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
> + * frequency = PCLK / (2 * (GPIO254[31:16] + 1))
> + * frequency * 2 * (GPIO254[31:16] + 1) = PCLK
> + * GPIO254[31:16] = PCLK / (frequency * 2) - 1
> + */
> + if (sgpio_freq == 0)
> + return -EINVAL;
> +
> + sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
> +
> + if (sgpio_clk_div > (1 << 16) - 1)
> + return -EINVAL;
> +
> + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> + ASPEED_SGPIO_ENABLE,
> + gpio->base + ASPEED_SGPIO_CTRL);
> +
> + spin_lock_init(&gpio->lock);
> +
> + gpio->chip.parent = &pdev->dev;
> + gpio->chip.ngpio = nr_gpios;
> + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> + gpio->chip.request = NULL;
> + gpio->chip.free = NULL;
> + gpio->chip.get = aspeed_sgpio_get;
> + gpio->chip.set = aspeed_sgpio_set;
> + gpio->chip.set_config = NULL;
> + gpio->chip.label = dev_name(&pdev->dev);
> + gpio->chip.base = ARCH_NR_GPIOS - MAX_NR_SGPIO;
Please set this to -1 so the base is chosen for you by the gpio subsystem.
> +
> + /* set all SGPIO pins as input. */
> + memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in));
You've used devm_kzalloc() above, so the memory is already zeroed. You can
remove the memset(), maybe just shift the comment to the devm_kzalloc().
Other than these small issues, looks good to me. Thanks for iterating on it.
Andrew
> +
> + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> + if (rc < 0)
> + return rc;
> +
> + return aspeed_sgpio_setup_irqs(gpio, pdev);
> +}
> +
> +static struct platform_driver aspeed_sgpio_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = aspeed_sgpio_of_table,
> + },
> +};
> +
> +module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
> +MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
>
^ permalink raw reply
* [PATCH 2/3 v4] ARM: dts: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-17 21:30 UTC (permalink / raw)
To: linux-aspeed
Add SGPIO driver support for Aspeed AST2500 SoC.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
drivers/gpio/sgpio-aspeed.c | 518 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 518 insertions(+)
create mode 100644 drivers/gpio/sgpio-aspeed.c
diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
new file mode 100644
index 0000000..715052c
--- /dev/null
+++ b/drivers/gpio/sgpio-aspeed.c
@@ -0,0 +1,518 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 American Megatrends International LLC.
+ *
+ * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/gpio/aspeed.h>
+#include <linux/hashtable.h>
+#include <linux/bitfield.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/gpio.h>
+
+#define MAX_NR_SGPIO 80
+
+#define ASPEED_SGPIO_CTRL 0x54
+
+#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
+#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
+#define ASPEED_SGPIO_ENABLE BIT(0)
+
+struct aspeed_sgpio {
+ struct gpio_chip chip;
+ struct clk *pclk;
+ spinlock_t lock;
+ void __iomem *base;
+ uint32_t dir_in[3];
+ int irq;
+};
+
+struct aspeed_sgpio_bank {
+ uint16_t val_regs;
+ uint16_t rdata_reg;
+ uint16_t irq_regs;
+ const char names[4][3];
+};
+
+/*
+ * Note: The "value" register returns the input value when the GPIO is
+ * configured as an input.
+ *
+ * The "rdata" register returns the output value when the GPIO is
+ * configured as an output.
+ */
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
+
+enum aspeed_sgpio_reg {
+ reg_val,
+ reg_rdata,
+ reg_irq_enable,
+ reg_irq_type0,
+ reg_irq_type1,
+ reg_irq_type2,
+ reg_irq_status,
+};
+
+#define GPIO_VAL_VALUE 0x00
+#define GPIO_IRQ_ENABLE 0x00
+#define GPIO_IRQ_TYPE0 0x04
+#define GPIO_IRQ_TYPE1 0x08
+#define GPIO_IRQ_TYPE2 0x0C
+#define GPIO_IRQ_STATUS 0x10
+
+/* This will be resolved at compile time */
+static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
+ const struct aspeed_sgpio_bank *bank,
+ const enum aspeed_sgpio_reg reg)
+{
+ switch (reg) {
+ case reg_val:
+ return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
+ case reg_rdata:
+ return gpio->base + bank->rdata_reg;
+ case reg_irq_enable:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
+ case reg_irq_type0:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
+ case reg_irq_type1:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
+ case reg_irq_type2:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
+ case reg_irq_status:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
+ default:
+ /* acturally if code runs to here, it's an error case */
+ BUG_ON(1);
+ }
+}
+
+#define GPIO_BANK(x) ((x) >> 5)
+#define GPIO_OFFSET(x) ((x) & 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);
+
+ WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
+ return &aspeed_sgpio_banks[bank];
+}
+
+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;
+ rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return rc;
+}
+
+static void aspeed_sgpio_set(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);
+ unsigned long flags;
+ void __iomem *addr;
+ u32 reg = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_val);
+
+ if (val)
+ reg |= GPIO_BIT(offset);
+ else
+ reg &= ~GPIO_BIT(offset);
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+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;
+}
+
+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;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return 0;
+}
+
+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;
+
+}
+
+static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d,
+ struct aspeed_sgpio **gpio,
+ const struct aspeed_sgpio_bank **bank,
+ u32 *bit, int *offset)
+{
+ struct aspeed_sgpio *internal;
+
+ *offset = irqd_to_hwirq(d);
+ internal = irq_data_get_irq_chip_data(d);
+ WARN_ON(!internal);
+
+ *gpio = internal;
+ *bank = to_bank(*offset);
+ *bit = GPIO_BIT(*offset);
+}
+
+static void aspeed_sgpio_irq_ack(struct irq_data *d)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *status_addr;
+ int offset;
+ u32 bit;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+ status_addr = bank_reg(gpio, bank, reg_irq_status);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ iowrite32(bit, status_addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ u32 reg, bit;
+ void __iomem *addr;
+ int offset;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ addr = bank_reg(gpio, bank, reg_irq_enable);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ reg = ioread32(addr);
+ if (set)
+ reg |= bit;
+ else
+ reg &= ~bit;
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_mask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, false);
+}
+
+static void aspeed_sgpio_irq_unmask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, true);
+}
+
+static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
+{
+ u32 type0 = 0;
+ u32 type1 = 0;
+ u32 type2 = 0;
+ u32 bit, reg;
+ const struct aspeed_sgpio_bank *bank;
+ irq_flow_handler_t handler;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *addr;
+ int offset;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_BOTH:
+ type2 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_RISING:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_FALLING:
+ handler = handle_edge_irq;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_LEVEL_LOW:
+ type1 |= bit;
+ handler = handle_level_irq;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_irq_type0);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type0;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type1);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type1;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type2);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type2;
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ irq_set_handler_locked(d, handler);
+
+ return 0;
+}
+
+static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *ic = irq_desc_get_chip(desc);
+ struct aspeed_sgpio *data = gpiochip_get_data(gc);
+ unsigned int i, p, girq;
+ unsigned long reg;
+
+ chained_irq_enter(ic, desc);
+
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
+
+ reg = ioread32(bank_reg(data, bank, reg_irq_status));
+
+ for_each_set_bit(p, ®, 32) {
+ girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
+ generic_handle_irq(girq);
+ }
+
+ }
+
+ chained_irq_exit(ic, desc);
+}
+
+static struct irq_chip aspeed_sgpio_irqchip = {
+ .name = "aspeed-sgpio",
+ .irq_ack = aspeed_sgpio_irq_ack,
+ .irq_mask = aspeed_sgpio_irq_mask,
+ .irq_unmask = aspeed_sgpio_irq_unmask,
+ .irq_set_type = aspeed_sgpio_set_type,
+};
+
+static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
+ struct platform_device *pdev)
+{
+ int rc, i;
+ const struct aspeed_sgpio_bank *bank;
+
+ rc = platform_get_irq(pdev, 0);
+ if (rc < 0)
+ return rc;
+
+ gpio->irq = rc;
+
+ /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* disable irq enable bits */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
+ /* clear status bits */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
+ }
+
+ rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
+ 0, handle_bad_irq, IRQ_TYPE_NONE);
+ if (rc) {
+ dev_info(&pdev->dev, "Could not add irqchip\n");
+ return rc;
+ }
+
+ gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
+ gpio->irq, aspeed_sgpio_irq_handler);
+
+ /* set IRQ settings and Enable Interrupt */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* set falling or level-low irq */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
+ /* trigger type is edge */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
+ /* dual edge trigger mode. */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
+ /* enable irq */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
+ }
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_sgpio_of_table[] = {
+ { .compatible = "aspeed,ast2400-sgpio" },
+ { .compatible = "aspeed,ast2500-sgpio" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
+
+static int __init aspeed_sgpio_probe(struct platform_device *pdev)
+{
+ struct aspeed_sgpio *gpio;
+ u32 nr_gpios, sgpio_freq, sgpio_clk_div;
+ int rc;
+ unsigned long apb_freq;
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(gpio->base))
+ return PTR_ERR(gpio->base);
+
+ rc = of_property_read_u32(pdev->dev.of_node, "nr-gpios", &nr_gpios);
+ if ((rc < 0) || (nr_gpios > MAX_NR_SGPIO)) {
+ dev_err(&pdev->dev, "Could not read nr-gpios property\n");
+ return -EINVAL;
+ }
+
+ rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+ return -EINVAL;
+ }
+
+ gpio->pclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(gpio->pclk)) {
+ dev_err(&pdev->dev, "devm_clk_get failed\n");
+ return PTR_ERR(gpio->pclk);
+ }
+
+ apb_freq = clk_get_rate(gpio->pclk);
+
+ /*
+ * From the datasheet,
+ * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
+ * period = 2 * (GPIO254[31:16] + 1) / PCLK
+ * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
+ * frequency = PCLK / (2 * (GPIO254[31:16] + 1))
+ * frequency * 2 * (GPIO254[31:16] + 1) = PCLK
+ * GPIO254[31:16] = PCLK / (frequency * 2) - 1
+ */
+ if (sgpio_freq == 0)
+ return -EINVAL;
+
+ sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
+
+ if (sgpio_clk_div > (1 << 16) - 1)
+ return -EINVAL;
+
+ iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
+ FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
+ ASPEED_SGPIO_ENABLE,
+ gpio->base + ASPEED_SGPIO_CTRL);
+
+ spin_lock_init(&gpio->lock);
+
+ gpio->chip.parent = &pdev->dev;
+ gpio->chip.ngpio = nr_gpios;
+ gpio->chip.direction_input = aspeed_sgpio_dir_in;
+ gpio->chip.direction_output = aspeed_sgpio_dir_out;
+ gpio->chip.get_direction = aspeed_sgpio_get_direction;
+ gpio->chip.request = NULL;
+ gpio->chip.free = NULL;
+ gpio->chip.get = aspeed_sgpio_get;
+ gpio->chip.set = aspeed_sgpio_set;
+ gpio->chip.set_config = NULL;
+ gpio->chip.label = dev_name(&pdev->dev);
+ gpio->chip.base = ARCH_NR_GPIOS - MAX_NR_SGPIO;
+
+ /* set all SGPIO pins as input. */
+ memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in));
+
+ rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+ if (rc < 0)
+ return rc;
+
+ return aspeed_sgpio_setup_irqs(gpio, pdev);
+}
+
+static struct platform_driver aspeed_sgpio_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = aspeed_sgpio_of_table,
+ },
+};
+
+module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
+MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* [PATCH 2/3 v3] dt-bindings: gpio: aspeed: Add SGPIO support
From: Hongwei Zhang @ 2019-07-17 21:30 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <91ebb817-3cc6-4b87-8c2b-cfcd66f4c284@www.fastmail.com>
Hello Andrew,
Thanks for your review, please find the v4 in separate email. We merged all your suggestion in v4.
Best Regards,
--Hongwei
-----Original Message-----
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Tuesday, July 16, 2019 11:26 PM
> To: Hongwei Zhang; Bartosz Golaszewski; Joel Stanley; Linus Walleij
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org;
> linux-kernel at vger.kernel.org
> Subject: Re: [PATCH 2/3 v3] ARM: dts: aspeed: Add SGPIO driver
>
> Hello Hongwei,
>
> Please send patches and feedback on prior iterations separately. Please send the output of `git format-
> patch ...`directly; format-patch spits the patch out in email form ready to go and can be fed straight to
> `git send-email`.
>
> On Wed, 17 Jul 2019, at 06:54, Hongwei Zhang wrote:
> > Add SGPIO driver support for Aspeed AST2500 SoC.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> > drivers/gpio/sgpio-aspeed.c | 487
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 487 insertions(+)
> > create mode 100644 drivers/gpio/sgpio-aspeed.c
> >
> > diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> > new file mode 100644 index 0000000..ade2cb7
> > --- /dev/null
> > +++ b/drivers/gpio/sgpio-aspeed.c
> > @@ -0,0 +1,487 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019 American Megatrends International LLC.
> > + *
> > + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in> */
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/gpio/aspeed.h>
>
> linux/gpio/aspeed.h is specific to the parallel GPIO driver, please drop this include.
>
Removed it.
> > +#include <linux/hashtable.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/init.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pinctrl/consumer.h>
>
> This driver doesn't have any direct interaction with pinctrl, so I think we can remove this header
>
Removed it.
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +#include <linux/gpio.h>
> > +
> > +#define MAX_NR_SGPIO 80
> > +
> > +#define ASPEED_SGPIO_CTRL 0x54
> > +
> > +#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
> > +#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
> > +#define ASPEED_SGPIO_ENABLE BIT(0)
> > +
> > +// default sgpio direction is input.
> > +static uint32_t sgpio_dir_val[3] = {0xffffffff, 0xffffffff,
> > +0xffffffff
> > };
>
> Why not make it a member of struct aspeed_sgpio (below)? I'd prefer we encode the comment in the
> variable name as well, e.g.
> sgpio_dir_in`- this way when reading the code that uses it we know which bit state means what (set is
> input, clear is output).
>
Done.
> > +
> > +struct aspeed_sgpio {
> > + struct gpio_chip chip;
> > + struct clk *pclk;
> > + spinlock_t lock;
> > + void __iomem *base;
> > + int irq;
> > +};
> > +
> > +struct aspeed_sgpio_bank {
> > + uint16_t val_regs;
> > + uint16_t rdata_reg;
> > + uint16_t irq_regs;
> > + const char names[4][3];
> > +};
> > +
> > +/*
> > + * Note: The "value" register returns the input value sampled on the
> > + * line even when the GPIO is configured as an output. Since
> > + * that input goes through synchronizers, writing, then reading
> > + * back may not return the written value right away.
>
> The paragraph above is somewhat specific to the parallel GPIO driver.
> It would be good to rework it for the context of the SGPIO driver.
> Documenting the split of the "value" and "rdata" register is a good thing.
>
> > + *
> > + * The "rdata" register returns the content of the write latch
> > + * and thus can be used to read back what was last written
> > + * reliably.
> > + */
> > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> > + {
> > + .val_regs = 0x0000,
> > + .rdata_reg = 0x0070,
> > + .irq_regs = 0x0004,
> > + .names = { "A", "B", "C", "D" },
> > + },
> > + {
> > + .val_regs = 0x001C,
> > + .rdata_reg = 0x0074,
> > + .irq_regs = 0x0020,
> > + .names = { "E", "F", "G", "H" },
> > + },
> > + {
> > + .val_regs = 0x0038,
> > + .rdata_reg = 0x0078,
> > + .irq_regs = 0x003C,
> > + .names = { "I", "J" },
> > + },
> > +};
> > +
> > +enum aspeed_sgpio_reg {
> > + reg_val,
> > + reg_rdata,
> > + reg_irq_enable,
> > + reg_irq_type0,
> > + reg_irq_type1,
> > + reg_irq_type2,
> > + reg_irq_status,
> > +};
> > +
> > +#define GPIO_VAL_VALUE 0x00
> > +#define GPIO_VAL_DIR 0x04
> > +#define GPIO_IRQ_ENABLE 0x00
> > +#define GPIO_IRQ_TYPE0 0x04
> > +#define GPIO_IRQ_TYPE1 0x08
> > +#define GPIO_IRQ_TYPE2 0x0C
> > +#define GPIO_IRQ_STATUS 0x10
> > +
> > +/* This will be resolved at compile time */ static inline void
> > +__iomem *bank_reg(struct aspeed_sgpio *gpio,
> > + const struct aspeed_sgpio_bank *bank,
> > + const enum aspeed_sgpio_reg reg) {
> > + switch (reg) {
> > + case reg_val:
> > + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> > + case reg_rdata:
> > + return gpio->base + bank->rdata_reg;
> > + case reg_irq_enable:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> > + case reg_irq_type0:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> > + case reg_irq_type1:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> > + case reg_irq_type2:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> > + case reg_irq_status:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> > + default:
> > + /* acturally if code runs to here, it's an error case */
> > + BUG_ON(1);
> > + }
> > +}
> > +
> > +#define GPIO_BANK(x) ((x) >> 5)
> > +#define GPIO_OFFSET(x) ((x) & 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);
> > +
> > + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> > + return &aspeed_sgpio_banks[bank];
> > +}
> > +
> > +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);
> > +
> > + if (sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset))
> > + return !!(ioread32(bank_reg(gpio, bank, reg_val)) &
> > GPIO_BIT(offset));
> > + else
> > + return !!(ioread32(bank_reg(gpio, bank, reg_rdata)) &
> > GPIO_BIT(offset));
>
> We don't need the else because we return from the body of the true case,
> and this could be written in a less redundant fashion. Also we need to do
> the read under gpio.lock for consistency with aspeed_sgpio_set().
>
> enum aspeed_sgpio_reg from;
> unsigned long flags;
> bool input;
> int rc;
>
> ...
>
> spin_lock_irqsave(&gpio->lock, flags);
> input = sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset);
> from = input ? reg_val : reg_rdata;
> rc = !!(ioread32(bank_reg(gpio, bank, from)) & GPIO_BIT(offset));
> spin_unlock_irqrestore(&gpio->lock, flags);
>
> return rc;
>
Updated code accordingly.
> > +
> > +}
> > +
> > +static void aspeed_sgpio_set(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);
> > + unsigned long flags;
> > + void __iomem *addr;
> > + u32 reg = 0;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + addr = bank_reg(gpio, bank, reg_val);
> > +
> > + if (val)
> > + reg |= GPIO_BIT(offset);
> > + else
> > + reg &= ~GPIO_BIT(offset);
> > +
> > + iowrite32(reg, addr);
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +}
> > +
> > +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int
> > offset)
> > +{
> > + sgpio_dir_val[GPIO_BANK(offset)] |= GPIO_BIT(offset);
>
> Also do all manipulations of sgpio_dir_val under the spinlock.
>
Added spinlock as you suggested.
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> > offset, int val)
> > +{
> > + sgpio_dir_val[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
>
> Again here.
>
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> > int offset)
> > +{
> > + return sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset);
>
> Again here.
>
> > +
> > +}
> > +
> > +static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
> > + struct aspeed_sgpio **gpio,
> > + const struct aspeed_sgpio_bank **bank,
> > + u32 *bit, int *offset)
> > +{
> > + struct aspeed_sgpio *internal;
> > +
> > + *offset = irqd_to_hwirq(d);
> > +
> > + internal = irq_data_get_irq_chip_data(d);
> > +
> > + *gpio = internal;
> > + *bank = to_bank(*offset);
> > + *bit = GPIO_BIT(*offset);
> > +
> > + return 0;
>
> It looks like this function could be a void function instead, and we
> could eliminate error checking from the callsites. If you're feeling
> paranoid you could `WARN_ON(!internal);` after the call to
> `irq_data_get_irq_chip_data(d)`.
>
Updated.
> > +}
> > +
> > +static void aspeed_sgpio_irq_ack(struct irq_data *d)
> > +{
> > + const struct aspeed_sgpio_bank *bank;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + void __iomem *status_addr;
> > + int rc, offset;
> > + u32 bit;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return;
> > +
> > + status_addr = bank_reg(gpio, bank, reg_irq_status);
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + iowrite32(bit, status_addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +}
> > +
> > +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
> > +{
> > + const struct aspeed_sgpio_bank *bank;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + u32 reg, bit;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return;
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_enable);
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + reg = ioread32(addr);
> > + if (set)
> > + reg |= bit;
> > + else
> > + reg &= ~bit;
> > +
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +}
> > +
> > +static void aspeed_sgpio_irq_mask(struct irq_data *d)
> > +{
> > + aspeed_sgpio_irq_set_mask(d, false);
> > +}
> > +
> > +static void aspeed_sgpio_irq_unmask(struct irq_data *d)
> > +{
> > + aspeed_sgpio_irq_set_mask(d, true);
> > +}
> > +
> > +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
> > +{
> > + u32 type0 = 0;
> > + u32 type1 = 0;
> > + u32 type2 = 0;
> > + u32 bit, reg;
> > + const struct aspeed_sgpio_bank *bank;
> > + irq_flow_handler_t handler;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return -EINVAL;
> > +
> > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_EDGE_BOTH:
> > + type2 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_EDGE_RISING:
> > + type0 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_EDGE_FALLING:
> > + handler = handle_edge_irq;
> > + break;
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + type0 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_LEVEL_LOW:
> > + type1 |= bit;
> > + handler = handle_level_irq;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type0);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type0;
> > + iowrite32(reg, addr);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type1);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type1;
> > + iowrite32(reg, addr);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type2);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type2;
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + irq_set_handler_locked(d, handler);
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
> > +{
> > + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *ic = irq_desc_get_chip(desc);
> > + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> > + unsigned int i, p, girq;
> > + unsigned long reg;
> > +
> > + chained_irq_enter(ic, desc);
> > +
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> > +
> > + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> > +
> > + for_each_set_bit(p, ®, 32) {
> > + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> > + generic_handle_irq(girq);
> > + }
> > +
> > + }
> > +
> > + chained_irq_exit(ic, desc);
> > +}
> > +
> > +static struct irq_chip aspeed_sgpio_irqchip = {
> > + .name = "aspeed-sgpio",
> > + .irq_ack = aspeed_sgpio_irq_ack,
> > + .irq_mask = aspeed_sgpio_irq_mask,
> > + .irq_unmask = aspeed_sgpio_irq_unmask,
> > + .irq_set_type = aspeed_sgpio_set_type,
> > +};
> > +
> > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> > + struct platform_device *pdev)
> > +{
> > + int rc, i;
> > + const struct aspeed_sgpio_bank *bank;
> > +
> > + rc = platform_get_irq(pdev, 0);
> > + if (rc < 0)
> > + return rc;
> > +
> > + gpio->irq = rc;
> > +
> > + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> > Pins. */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + bank = &aspeed_sgpio_banks[i];
> > + /* disable irq enable bits */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> > + /* clear status bits */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> > + }
> > +
> > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> > + 0, handle_bad_irq, IRQ_TYPE_NONE);
> > + if (rc) {
> > + dev_info(&pdev->dev, "Could not add irqchip\n");
> > + return rc;
> > + }
> > +
> > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> > + gpio->irq, aspeed_sgpio_irq_handler);
> > +
> > + /* set IRQ settings and Enable Interrupt */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + bank = &aspeed_sgpio_banks[i];
> > + /* set falling or level-low irq */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> > + /* trigger type is edge */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> > + /* dual edge trigger mode. */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> > + /* enable irq */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id aspeed_sgpio_of_table[] = {
> > + { .compatible = "aspeed,ast2400-sgpio" },
> > + { .compatible = "aspeed,ast2500-sgpio" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> > +
> > +static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> > +{
> > + struct aspeed_sgpio *gpio;
> > + struct resource *res;
> > + u32 nr_gpios, sgpio_freq;
> > + int rc;
> > + u16 sgpio_clk_div;
>
> Lets make this a u32 as it will help error detection below.
>
> > + unsigned long apb_freq;
> > +
> > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> > + if (!gpio)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + gpio->base = devm_ioremap_resource(&pdev->dev, res);
>
> Please use devm_platform_ioremap_resource() here.
>
> > + if (IS_ERR(gpio->base))
> > + return PTR_ERR(gpio->base);
> > +
> > + rc = of_property_read_u32(pdev->dev.of_node, "nr-gpios", &nr_gpios);
> > + if ((rc < 0) || (nr_gpios > MAX_NR_SGPIO))
> > + nr_gpios = MAX_NR_SGPIO;
>
> This is an error state, not something we should paper over. This should be
> `return -EINVAL;`
>
Updated.
> > +
> > + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency",
> > &sgpio_freq);
> > + if (rc < 0) {
> > + dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> > + sgpio_freq = 12000000;
>
> Again, I suggested previously that this is a required property, not optional.
> As such there should not be fall-back code here. This is another case of
> `return -EINVAL;`.
>
Updated.
> > + }
> > +
> > + gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(gpio->pclk)) {
> > + dev_err(&pdev->dev, "devm_clk_get failed\n");
> > + return PTR_ERR(gpio->pclk);
> > + }
> > +
> > + apb_freq = clk_get_rate(gpio->pclk);
> > + sgpio_clk_div = 2 * ((apb_freq % sgpio_freq == 0) ?
> > + (apb_freq / sgpio_freq) - 1 : (apb_freq / sgpio_freq));
>
> This calculation seems overly complex and possibly incorrect (need to
> multiply the denominator or divide the result, not multiply the result)?
>
> From the datasheet, the SGPM clock period calculation is:
>
> period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
>
> rearranging:
>
> period = 2 * (GPIO254[31:16] + 1) / PCLK
>
> Converting back to bus frequency:
>
> frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
>
> Which rearranges to:
>
> frequency = PCLK / (2 * (GPIO254[31:16] + 1))
>
> Extracting GPIO254[31:16] in terms of PCLK / frequency from above:
>
> frequency * 2 * (GPIO254[31:16] + 1) = PCLK
>
> And so:
>
> GPIO254[31:16] = PCLK / (frequency * 2) - 1
>
> From that, the code should look something like:
>
> if (sgpio_freq == 0)
> return -EINVAL;
>
> sgpio_clk_div = apb_freq / (sgpio_freq * 2) - 1;
>
> if (sgpio_clk_div > (1 << 16) - 1)
> return -EINVAL;
>
> This seems to work at the extremes (sgpio_clk_div = 0 and
> sgpio_clk_div = 65535), and we get 32766.99 on a round-trip of
> the divider value 32768, which if we truncate gives an error of 0.023Hz
> with an APB of 24.75MHz (value reported from one of our boards).
>
> Andrew
>
Thanks for the formula and detailed suggestion, really appreciated.
> > + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> > + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> > + ASPEED_SGPIO_ENABLE,
> > + gpio->base + ASPEED_SGPIO_CTRL);
> > +
> > + spin_lock_init(&gpio->lock);
> > +
> > + gpio->chip.parent = &pdev->dev;
> > + gpio->chip.ngpio = nr_gpios;
> > + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> > + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> > + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> > + gpio->chip.request = NULL;
> > + gpio->chip.free = NULL;
> > + gpio->chip.get = aspeed_sgpio_get;
> > + gpio->chip.set = aspeed_sgpio_set;
> > + gpio->chip.set_config = NULL;
> > + gpio->chip.label = dev_name(&pdev->dev);
> > + gpio->chip.base = ARCH_NR_GPIOS - MAX_NR_SGPIO;
> > +
> > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> > + if (rc < 0)
> > + return rc;
> > +
> > + return aspeed_sgpio_setup_irqs(gpio, pdev);
> > +}
> > +
> > +static struct platform_driver aspeed_sgpio_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .of_match_table = aspeed_sgpio_of_table,
> > + },
> > +};
> > +
> > +module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
> > +MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.7.4
^ permalink raw reply
* [PATCH 2/3 v4] dt-bindings: gpio: aspeed: Add SGPIO support
From: Hongwei Zhang @ 2019-07-17 20:12 UTC (permalink / raw)
To: linux-aspeed
Add bindings to support SGPIO on AST2400 or AST2500.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
.../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
new file mode 100644
index 0000000..2d6305e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
@@ -0,0 +1,55 @@
+Aspeed SGPIO controller Device Tree Bindings
+-------------------------------------------
+
+This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80 full
+featured Serial GPIOs. Each of the Serial GPIO pins can be programmed to
+support the following options:
+- Support interrupt option for each input port and various interrupt
+ sensitivity option (level-high, level-low, edge-high, edge-low)
+- Support reset tolerance option for each output port
+- Directly connected to APB bus and its shift clock is from APB bus clock
+ divided by a programmable value.
+- Co-work with external signal-chained TTL components (74LV165/74LV595)
+
+
+Required properties:
+
+- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
+
+- #gpio-cells : Should be two
+ - First cell is the GPIO line number
+ - Second cell is used to specify optional
+ parameters (unused)
+
+- reg : Address and length of the register set for the device
+- gpio-controller : Marks the device node as a GPIO controller
+- interrupts : Interrupt specifier (see interrupt bindings for
+ details)
+
+- interrupt-controller : Mark the GPIO controller as an interrupt-controller
+
+- nr-gpios : number of GPIO pins to serialise.
+ (should be multiple of 8, up to 80 pins)
+
+- clocks : A phandle to the APB clock for SGPM clock division
+
+- bus-frequency : SGPM CLK frequency
+
+
+The sgpio and interrupt properties are further described in their respective bindings documentation:
+
+- Documentation/devicetree/bindings/sgpio/gpio.txt
+- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+
+ Example:
+ sgpio: sgpio at 1e780200 {
+ #gpio-cells = <2>;
+ compatible = "aspeed,ast2500-sgpio";
+ gpio-controller;
+ interrupts = <40>;
+ reg = <0x1e780200 0x0100>;
+ clocks = <&syscon ASPEED_CLK_APB>;
+ interrupt-controller;
+ nr-gpios = <8>;
+ bus-frequency = <12000000>;
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Rob Herring @ 2019-07-17 13:43 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <e7b472a8-73ae-4f39-a3e4-9e2d9dbcd01e@www.fastmail.com>
On Tue, Jul 16, 2019 at 9:58 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote:
> > On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > >
> > >
> > > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> > > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > >
> > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > > > data bus if only a single slot is enabled.
> > > > >
> > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > > ---
> > > > > In v2:
> > > > >
> > > > > * Rename to aspeed,sdhci.yaml
> > > > > * Rename sd-controller compatible
> > > > > * Add `maxItems: 1` for reg properties
> > > > > * Move sdhci subnode description to patternProperties
> > > > > * Drop sdhci compatible requirement
> > > > > * #address-cells and #size-cells are required
> > > > > * Prevent additional properties
> > > > > * Implement explicit ranges in example
> > > > > * Remove slot property
> > > > >
> > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > > > 1 file changed, 90 insertions(+)
> > > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..67a691c3348c
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > @@ -0,0 +1,90 @@
> > > > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: ASPEED SD/SDIO/eMMC Controller
> > > > > +
> > > > > +maintainers:
> > > > > + - Andrew Jeffery <andrew@aj.id.au>
> > > > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > > > +
> > > > > +description: |+
> > > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > > > + only a single slot is enabled.
> > > > > +
> > > > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > > > + the slots are dependent on the common configuration area, they are described
> > > > > + as child nodes.
> > > > > +
> > > > > +properties:
> > > > > + compatible:
> > > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> > > >
> > > > This is actually a list of 4 strings. Please reformat to 1 per line.
> > >
> > > On reflection that's obvious, but also a somewhat subtle interaction with the
> > > preference for no quotes (the obvious caveat being "except where required").
> >
> > It wasn't something I'd run into before. I'm working on a check, but
> > unfortunately we can only check for quotes not needed and can't check
> > for missing quotes.
> >
> > > Thanks for pointing it out.
> > >
> > > I have been running `make dt_binding_check` and `make dtbs_check` over
> > > these, looks like I need to up my game a bit though. Do you do additional things
> > > in your workflow?
> >
> > That should have thrown the warnings. If you aren't seeing those, do
> > you have dtschema package installed (see
> > Documentation/devicetree/writing-schema.md)?
>
> I do have it installed, but as mentioned previously there's a fair few
> warnings emitted currently by the Aspeed devicetrees, so it might have
> got lost in the noise. I've started to clean that up, though probably need
> some direction there too.
>
> Separately I'm currently trying to track down an issue where I get errors
> on the Aspeed dts cpu nodes about failing to match the riscv CPU
> compatibles, it seems dt-validate isn't finding the ARM CPU compatible
> strings. It feels more annoying to track down that I'd like.
There's a fix in today's linux-next for that and it should be in
Linus' tree in a few days.
Rob
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Andrew Jeffery @ 2019-07-17 3:57 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAL_JsqKMo_uv4Ur-D4NaUXk94hGJeRt5fg+0998dDjJCTgumGg@mail.gmail.com>
On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote:
> On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > > data bus if only a single slot is enabled.
> > > >
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > ---
> > > > In v2:
> > > >
> > > > * Rename to aspeed,sdhci.yaml
> > > > * Rename sd-controller compatible
> > > > * Add `maxItems: 1` for reg properties
> > > > * Move sdhci subnode description to patternProperties
> > > > * Drop sdhci compatible requirement
> > > > * #address-cells and #size-cells are required
> > > > * Prevent additional properties
> > > > * Implement explicit ranges in example
> > > > * Remove slot property
> > > >
> > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > > 1 file changed, 90 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > new file mode 100644
> > > > index 000000000000..67a691c3348c
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > @@ -0,0 +1,90 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: ASPEED SD/SDIO/eMMC Controller
> > > > +
> > > > +maintainers:
> > > > + - Andrew Jeffery <andrew@aj.id.au>
> > > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > > +
> > > > +description: |+
> > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > > + only a single slot is enabled.
> > > > +
> > > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > > + the slots are dependent on the common configuration area, they are described
> > > > + as child nodes.
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> > >
> > > This is actually a list of 4 strings. Please reformat to 1 per line.
> >
> > On reflection that's obvious, but also a somewhat subtle interaction with the
> > preference for no quotes (the obvious caveat being "except where required").
>
> It wasn't something I'd run into before. I'm working on a check, but
> unfortunately we can only check for quotes not needed and can't check
> for missing quotes.
>
> > Thanks for pointing it out.
> >
> > I have been running `make dt_binding_check` and `make dtbs_check` over
> > these, looks like I need to up my game a bit though. Do you do additional things
> > in your workflow?
>
> That should have thrown the warnings. If you aren't seeing those, do
> you have dtschema package installed (see
> Documentation/devicetree/writing-schema.md)?
I do have it installed, but as mentioned previously there's a fair few
warnings emitted currently by the Aspeed devicetrees, so it might have
got lost in the noise. I've started to clean that up, though probably need
some direction there too.
Separately I'm currently trying to track down an issue where I get errors
on the Aspeed dts cpu nodes about failing to match the riscv CPU
compatibles, it seems dt-validate isn't finding the ARM CPU compatible
strings. It feels more annoying to track down that I'd like.
> Or it could be erroring
> out on something else first. There's a few breakages that I'm trying
> to fix.
Okay. I'll keep an eye on the dt-schema repo.
Cheers,
Andrew
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Andrew Jeffery @ 2019-07-17 3:51 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAL_Jsq+AJDNZ-676iP=vv6G-pjWqBJyZ3bJ26o7i=c=KWbozSw@mail.gmail.com>
On Wed, 17 Jul 2019, at 00:35, Rob Herring wrote:
> On Mon, Jul 15, 2019 at 5:17 PM Joel Stanley <joel@jms.id.au> wrote:
> >
> > On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
> > >
> > > The Aspeed pinctl schema have errors in the 'compatible' schema:
> > >
> > > Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> > > properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> > > Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> > > properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
> > >
> > > Flow style sequences have to be quoted if the vales contain ','. Fix
> > > this by using the more common one line per entry formatting.
> >
> > >
> > > properties:
> > > compatible:
> > > - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> > > + enum:
> > > + - aspeed,ast2400-pinctrl
> > > + - aspeed,g4-pinctrl
> >
> > Thanks for the fix. However, we've standardised on the first form for
> > all of our device trees, so we can drop the second compatible string
> > from the bindings.
>
> Doing that would introduce validation warnings until the dts file is
> updated. So we still need this change until that happens.
My series takes care of that.
Andrew
^ permalink raw reply
* [PATCH 2/3 v3] dt-bindings: gpio: aspeed: Add SGPIO support
From: Andrew Jeffery @ 2019-07-17 3:49 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563313711-17961-1-git-send-email-hongweiz@ami.com>
Hello Hongwei,
On Wed, 17 Jul 2019, at 07:18, Hongwei Zhang wrote:
> Add bindings to support SGPIO on AST2400 or AST2500.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> new file mode 100644
> index 0000000..8c3a747
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> @@ -0,0 +1,55 @@
> +Aspeed SGPIO controller Device Tree Bindings
> +-------------------------------------------
> +
> +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80
> full
> +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed
> to
> +support the following options:
> +- Support interrupt option for each input port and various interrupt
> + sensitivity option (level-high, level-low, edge-high, edge-low)
> +- Support reset tolerance option for each output port
> +- Directly connected to APB bus and its shift clock is from APB bus
> clock
> + divided by a programmable value.
> +- Co-work with external signal-chained TTL components (74LV165/74LV595)
Nice description.
> +
> +
> +Required properties:
> +
> +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> +
> +- #gpio-cells : Should be two
> + - First cell is the GPIO line number
> + - Second cell is used to specify optional
> + parameters (unused)
> +
> +- reg : Address and length of the register set for the device
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- interrupts : Interrupt specifier (see interrupt bindings for
> + details)
> +
> +- interrupt-controller : Mark the GPIO controller as an
> interrupt-controller
> +
> +- nr-gpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins; 0 if not used)
It's unclear to me what you mean by "0 if not used" here. The property is
required, so its description in a devicetree should always have a non-zero
value of `status = "okay";`, as 0 is an invalid value according to the
datasheet (sensibly so). If `status = "disabled";` then it doesn't really
matter, which makes the comment not terribly useful.
> +
> +- clocks : A phandle to the APB clock for SGPM clock
> division
> +
> +- bus-frequency : SGPM CLK frequency, derived from APB bus clock by a
> programmable devisor
I'd leave off the parent clock information. Practically speaking it's probably
always going to be the APB clock, but who knows. From a devicetree writer's
perspective they just want to say "make it 7MHz" or whatever speed they,
and it shouldn't matter too much how we get there.
Finally, as mentioned on the driver patch, please send v4 without the history
at the bottom.
Cheers,
Andrew
> +
> +
> +The sgpio and interrupt properties are further described in their
> respective bindings documentation:
> +
> +- Documentation/devicetree/bindings/sgpio/gpio.txt
> +- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> + Example:
> + sgpio at 1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + nr-gpios = <8>;
> + bus-frequency = <12000000>;
> + };
> --
> 2.7.4
>
>
> Thanks Andrew, please see above v3 and inline comments at below.
> --Hongwei
>
> > From: Andrew Jeffery <andrew@aj.id.au>
> > Sent: Sunday, July 14, 2019 10:25 PM
> > To: Hongwei Zhang; Joel Stanley; Linus Walleij; devicetree at vger.kernel.org
> > Cc: Rob Herring; Mark Rutland; Bartosz Golaszewski; linux-aspeed at lists.ozlabs.org; linux-
> > kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-gpio at vger.kernel.org
> > Subject: Re: [PATCH 2/3 v2] dt-bindings: gpio: aspeed: Add SGPIO support
> >
> > Hello Hongwei,
> >
> > On Sat, 13 Jul 2019, at 05:44, Hongwei Zhang wrote:
> > > Add bindings to support SGPIO on AST2400 or AST2500.
> > >
> > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > > ---
> > > .../devicetree/bindings/gpio/sgpio-aspeed.txt | 43 ++++++++++++++++++++++
> > > 1 file changed, 43 insertions(+)
> > > create mode 100755
> > > Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > new file mode 100755
> > > index 0000000..3ae2b79
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > > @@ -0,0 +1,43 @@
> > > +Aspeed SGPIO controller Device Tree Bindings
> > > +-------------------------------------------
> > > +
> > > +Required properties:
> > > +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> > > +
> > > +- #gpio-cells : Should be two
> > > + - First cell is the GPIO line number
> > > + - Second cell is used to specify optional
> > > + parameters (unused)
> > > +
> > > +- reg : Address and length of the register set for the device
> > > +- gpio-controller : Marks the device node as a GPIO controller.
> > > +- interrupts : Interrupt specifier (see interrupt bindings for
> > > + details)
> > > +
> > > +- interrupt-controller : Mark the GPIO controller as an
> > > interrupt-controller
> > > +
> > > +- nr-gpios : number of GPIO pins to serialise. (should be multiple of
> > > 8, up to 80 pins)
> > > + if not specified, defaults to 80.
> >
> > This appears to be a statement about the driver implementation, but bindings documents are about
> > describing hardware. Reading the datasheet it actually appears the ASPEED SGPIO hardware comes up
> > in what is "technically" a forbidden state (equivalent to `nr-gpios = <0>;`), though the device is also
> > disabled at this point, so it's probably moot. The point is the true default value from a hardware
> > perspective is 0, not 80, so if we're going to talk about default values, 0 would be more appropriate.
> > However:
> >
> > You've also listed nr-gpios under the "Required properties" header, but the description suggests it's
> > optional. It's either one or the other, please lets be clear about it. On that front, lets make it nr-gpios
> > *not* optional (i.e. make it
> > required) thus force the specification of how many SGPIOs we want to emit on the bus. This value is
> > coupled to the platform design, so I don't think there's ever a scenario where we want nr-gpios to take a
> > default value.
> >
>
> Added some descriptions and updated nr-gpios, please see v3.
>
> > > +
> > > +- clocks : A phandle to the APB clock for SGPM clock
> > > division
> > > +
> > > +- bus-frequency : SGPM CLK frequency, if not specified defaults to 1
> > > MHz
> >
> > Again here with the default value - SGPM CLK period is derived from PCLK by the expression `period =
> > PCLK * 2 *(GPIO254[31:16] + 1)`, where GPIO254's initialisation state is `GPIO254[31:16] = 0`, which
> > gives a default SGPM bus frequency of PCLK / 2. This is likely not going to be 1MHz (more like ~12MHz).
> >
> > Lets just make the property required. That way we avoid any ambiguity about the bus frequency and
> > thus don't need words about defaults that turn out to be about the driver, not about the hardware.
> >
>
> updated, please see v3.
>
> > Finally, when updating patches in response to feedback, please send the full series again, and bump the
> > series version number. That way people can review a coherent set of patches and not have to hunt
> > around and (fail to) collate the correct combination. It makes it easier to say "Reviewed-by:" on your
> > patches :)
> >
> > Cheers,
> >
> > Andrew
>
^ permalink raw reply
* [PATCH 2/3 v3] ARM: dts: aspeed: Add SGPIO driver
From: Andrew Jeffery @ 2019-07-17 3:25 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563312271-17509-1-git-send-email-hongweiz@ami.com>
Hello Hongwei,
Please send patches and feedback on prior iterations separately. Please send the
output of `git format-patch ...`directly; format-patch spits the patch out in email
form ready to go and can be fed straight to `git send-email`.
On Wed, 17 Jul 2019, at 06:54, Hongwei Zhang wrote:
> Add SGPIO driver support for Aspeed AST2500 SoC.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> drivers/gpio/sgpio-aspeed.c | 487 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 487 insertions(+)
> create mode 100644 drivers/gpio/sgpio-aspeed.c
>
> diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> new file mode 100644
> index 0000000..ade2cb7
> --- /dev/null
> +++ b/drivers/gpio/sgpio-aspeed.c
> @@ -0,0 +1,487 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2019 American Megatrends International LLC.
> + *
> + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
> + */
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/aspeed.h>
linux/gpio/aspeed.h is specific to the parallel GPIO driver, please drop
this include.
> +#include <linux/hashtable.h>
> +#include <linux/bitfield.h>
> +#include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
This driver doesn't have any direct interaction with pinctrl, so I think
we can remove this header
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +#include <linux/gpio.h>
> +
> +#define MAX_NR_SGPIO 80
> +
> +#define ASPEED_SGPIO_CTRL 0x54
> +
> +#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
> +#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
> +#define ASPEED_SGPIO_ENABLE BIT(0)
> +
> +// default sgpio direction is input.
> +static uint32_t sgpio_dir_val[3] = {0xffffffff, 0xffffffff, 0xffffffff
> };
Why not make it a member of struct aspeed_sgpio (below)? I'd prefer
we encode the comment in the variable name as well, e.g.
sgpio_dir_in`- this way when reading the code that uses it we know
which bit state means what (set is input, clear is output).
> +
> +struct aspeed_sgpio {
> + struct gpio_chip chip;
> + struct clk *pclk;
> + spinlock_t lock;
> + void __iomem *base;
> + int irq;
> +};
> +
> +struct aspeed_sgpio_bank {
> + uint16_t val_regs;
> + uint16_t rdata_reg;
> + uint16_t irq_regs;
> + const char names[4][3];
> +};
> +
> +/*
> + * Note: The "value" register returns the input value sampled on the
> + * line even when the GPIO is configured as an output. Since
> + * that input goes through synchronizers, writing, then reading
> + * back may not return the written value right away.
The paragraph above is somewhat specific to the parallel GPIO driver.
It would be good to rework it for the context of the SGPIO driver.
Documenting the split of the "value" and "rdata" register is a good
thing.
> + *
> + * The "rdata" register returns the content of the write latch
> + * and thus can be used to read back what was last written
> + * reliably.
> + */
> +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> + {
> + .val_regs = 0x0000,
> + .rdata_reg = 0x0070,
> + .irq_regs = 0x0004,
> + .names = { "A", "B", "C", "D" },
> + },
> + {
> + .val_regs = 0x001C,
> + .rdata_reg = 0x0074,
> + .irq_regs = 0x0020,
> + .names = { "E", "F", "G", "H" },
> + },
> + {
> + .val_regs = 0x0038,
> + .rdata_reg = 0x0078,
> + .irq_regs = 0x003C,
> + .names = { "I", "J" },
> + },
> +};
> +
> +enum aspeed_sgpio_reg {
> + reg_val,
> + reg_rdata,
> + reg_irq_enable,
> + reg_irq_type0,
> + reg_irq_type1,
> + reg_irq_type2,
> + reg_irq_status,
> +};
> +
> +#define GPIO_VAL_VALUE 0x00
> +#define GPIO_VAL_DIR 0x04
> +#define GPIO_IRQ_ENABLE 0x00
> +#define GPIO_IRQ_TYPE0 0x04
> +#define GPIO_IRQ_TYPE1 0x08
> +#define GPIO_IRQ_TYPE2 0x0C
> +#define GPIO_IRQ_STATUS 0x10
> +
> +/* This will be resolved at compile time */
> +static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
> + const struct aspeed_sgpio_bank *bank,
> + const enum aspeed_sgpio_reg reg)
> +{
> + switch (reg) {
> + case reg_val:
> + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> + case reg_rdata:
> + return gpio->base + bank->rdata_reg;
> + case reg_irq_enable:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> + case reg_irq_type0:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> + case reg_irq_type1:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> + case reg_irq_type2:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> + case reg_irq_status:
> + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> + default:
> + /* acturally if code runs to here, it's an error case */
> + BUG_ON(1);
> + }
> +}
> +
> +#define GPIO_BANK(x) ((x) >> 5)
> +#define GPIO_OFFSET(x) ((x) & 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);
> +
> + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> + return &aspeed_sgpio_banks[bank];
> +}
> +
> +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);
> +
> + if (sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset))
> + return !!(ioread32(bank_reg(gpio, bank, reg_val)) &
> GPIO_BIT(offset));
> + else
> + return !!(ioread32(bank_reg(gpio, bank, reg_rdata)) &
> GPIO_BIT(offset));
We don't need the else because we return from the body of the true case,
and this could be written in a less redundant fashion. Also we need to do
the read under gpio.lock for consistency with aspeed_sgpio_set().
enum aspeed_sgpio_reg from;
unsigned long flags;
bool input;
int rc;
...
spin_lock_irqsave(&gpio->lock, flags);
input = sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset);
from = input ? reg_val : reg_rdata;
rc = !!(ioread32(bank_reg(gpio, bank, from)) & GPIO_BIT(offset));
spin_unlock_irqrestore(&gpio->lock, flags);
return rc;
> +
> +}
> +
> +static void aspeed_sgpio_set(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);
> + unsigned long flags;
> + void __iomem *addr;
> + u32 reg = 0;
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + addr = bank_reg(gpio, bank, reg_val);
> +
> + if (val)
> + reg |= GPIO_BIT(offset);
> + else
> + reg &= ~GPIO_BIT(offset);
> +
> + iowrite32(reg, addr);
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int
> offset)
> +{
> + sgpio_dir_val[GPIO_BANK(offset)] |= GPIO_BIT(offset);
Also do all manipulations of sgpio_dir_val under the spinlock.
> + return 0;
> +}
> +
> +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> offset, int val)
> +{
> + sgpio_dir_val[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
Again here.
> + return 0;
> +}
> +
> +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> int offset)
> +{
> + return sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset);
Again here.
> +
> +}
> +
> +static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
> + struct aspeed_sgpio **gpio,
> + const struct aspeed_sgpio_bank **bank,
> + u32 *bit, int *offset)
> +{
> + struct aspeed_sgpio *internal;
> +
> + *offset = irqd_to_hwirq(d);
> +
> + internal = irq_data_get_irq_chip_data(d);
> +
> + *gpio = internal;
> + *bank = to_bank(*offset);
> + *bit = GPIO_BIT(*offset);
> +
> + return 0;
It looks like this function could be a void function instead, and we
could eliminate error checking from the callsites. If you're feeling
paranoid you could `WARN_ON(!internal);` after the call to
`irq_data_get_irq_chip_data(d)`.
> +}
> +
> +static void aspeed_sgpio_irq_ack(struct irq_data *d)
> +{
> + const struct aspeed_sgpio_bank *bank;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + void __iomem *status_addr;
> + int rc, offset;
> + u32 bit;
> +
> + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> + if (rc)
> + return;
> +
> + status_addr = bank_reg(gpio, bank, reg_irq_status);
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + iowrite32(bit, status_addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
> +{
> + const struct aspeed_sgpio_bank *bank;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + u32 reg, bit;
> + void __iomem *addr;
> + int rc, offset;
> +
> + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> + if (rc)
> + return;
> +
> + addr = bank_reg(gpio, bank, reg_irq_enable);
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + reg = ioread32(addr);
> + if (set)
> + reg |= bit;
> + else
> + reg &= ~bit;
> +
> + iowrite32(reg, addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static void aspeed_sgpio_irq_mask(struct irq_data *d)
> +{
> + aspeed_sgpio_irq_set_mask(d, false);
> +}
> +
> +static void aspeed_sgpio_irq_unmask(struct irq_data *d)
> +{
> + aspeed_sgpio_irq_set_mask(d, true);
> +}
> +
> +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
> +{
> + u32 type0 = 0;
> + u32 type1 = 0;
> + u32 type2 = 0;
> + u32 bit, reg;
> + const struct aspeed_sgpio_bank *bank;
> + irq_flow_handler_t handler;
> + struct aspeed_sgpio *gpio;
> + unsigned long flags;
> + void __iomem *addr;
> + int rc, offset;
> +
> + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> + if (rc)
> + return -EINVAL;
> +
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_BOTH:
> + type2 |= bit;
> + /* fall through */
> + case IRQ_TYPE_EDGE_RISING:
> + type0 |= bit;
> + /* fall through */
> + case IRQ_TYPE_EDGE_FALLING:
> + handler = handle_edge_irq;
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + type0 |= bit;
> + /* fall through */
> + case IRQ_TYPE_LEVEL_LOW:
> + type1 |= bit;
> + handler = handle_level_irq;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type0);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type0;
> + iowrite32(reg, addr);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type1);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type1;
> + iowrite32(reg, addr);
> +
> + addr = bank_reg(gpio, bank, reg_irq_type2);
> + reg = ioread32(addr);
> + reg = (reg & ~bit) | type2;
> + iowrite32(reg, addr);
> +
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +
> + irq_set_handler_locked(d, handler);
> +
> + return 0;
> +}
> +
> +static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *ic = irq_desc_get_chip(desc);
> + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> + unsigned int i, p, girq;
> + unsigned long reg;
> +
> + chained_irq_enter(ic, desc);
> +
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> +
> + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> +
> + for_each_set_bit(p, ®, 32) {
> + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> + generic_handle_irq(girq);
> + }
> +
> + }
> +
> + chained_irq_exit(ic, desc);
> +}
> +
> +static struct irq_chip aspeed_sgpio_irqchip = {
> + .name = "aspeed-sgpio",
> + .irq_ack = aspeed_sgpio_irq_ack,
> + .irq_mask = aspeed_sgpio_irq_mask,
> + .irq_unmask = aspeed_sgpio_irq_unmask,
> + .irq_set_type = aspeed_sgpio_set_type,
> +};
> +
> +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> + struct platform_device *pdev)
> +{
> + int rc, i;
> + const struct aspeed_sgpio_bank *bank;
> +
> + rc = platform_get_irq(pdev, 0);
> + if (rc < 0)
> + return rc;
> +
> + gpio->irq = rc;
> +
> + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> Pins. */
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + bank = &aspeed_sgpio_banks[i];
> + /* disable irq enable bits */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> + /* clear status bits */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> + }
> +
> + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> + 0, handle_bad_irq, IRQ_TYPE_NONE);
> + if (rc) {
> + dev_info(&pdev->dev, "Could not add irqchip\n");
> + return rc;
> + }
> +
> + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> + gpio->irq, aspeed_sgpio_irq_handler);
> +
> + /* set IRQ settings and Enable Interrupt */
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + bank = &aspeed_sgpio_banks[i];
> + /* set falling or level-low irq */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> + /* trigger type is edge */
> + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> + /* dual edge trigger mode. */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> + /* enable irq */
> + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_sgpio_of_table[] = {
> + { .compatible = "aspeed,ast2400-sgpio" },
> + { .compatible = "aspeed,ast2500-sgpio" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> +
> +static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> +{
> + struct aspeed_sgpio *gpio;
> + struct resource *res;
> + u32 nr_gpios, sgpio_freq;
> + int rc;
> + u16 sgpio_clk_div;
Lets make this a u32 as it will help error detection below.
> + unsigned long apb_freq;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + gpio->base = devm_ioremap_resource(&pdev->dev, res);
Please use devm_platform_ioremap_resource() here.
> + if (IS_ERR(gpio->base))
> + return PTR_ERR(gpio->base);
> +
> + rc = of_property_read_u32(pdev->dev.of_node, "nr-gpios", &nr_gpios);
> + if ((rc < 0) || (nr_gpios > MAX_NR_SGPIO))
> + nr_gpios = MAX_NR_SGPIO;
This is an error state, not something we should paper over. This should be
`return -EINVAL;`
> +
> + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency",
> &sgpio_freq);
> + if (rc < 0) {
> + dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> + sgpio_freq = 12000000;
Again, I suggested previously that this is a required property, not optional.
As such there should not be fall-back code here. This is another case of
`return -EINVAL;`.
> + }
> +
> + gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(gpio->pclk)) {
> + dev_err(&pdev->dev, "devm_clk_get failed\n");
> + return PTR_ERR(gpio->pclk);
> + }
> +
> + apb_freq = clk_get_rate(gpio->pclk);
> + sgpio_clk_div = 2 * ((apb_freq % sgpio_freq == 0) ?
> + (apb_freq / sgpio_freq) - 1 : (apb_freq / sgpio_freq));
This calculation seems overly complex and possibly incorrect (need to
multiply the denominator or divide the result, not multiply the result)?
From the datasheet, the SGPM clock period calculation is:
period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
rearranging:
period = 2 * (GPIO254[31:16] + 1) / PCLK
Converting back to bus frequency:
frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
Which rearranges to:
frequency = PCLK / (2 * (GPIO254[31:16] + 1))
Extracting GPIO254[31:16] in terms of PCLK / frequency from above:
frequency * 2 * (GPIO254[31:16] + 1) = PCLK
And so:
GPIO254[31:16] = PCLK / (frequency * 2) - 1
From that, the code should look something like:
if (sgpio_freq == 0)
return -EINVAL;
sgpio_clk_div = apb_freq / (sgpio_freq * 2) - 1;
if (sgpio_clk_div > (1 << 16) - 1)
return -EINVAL;
This seems to work@the extremes (sgpio_clk_div = 0 and
sgpio_clk_div = 65535), and we get 32766.99 on a round-trip of
the divider value 32768, which if we truncate gives an error of 0.023Hz
with an APB of 24.75MHz (value reported from one of our boards).
Andrew
> + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> + ASPEED_SGPIO_ENABLE,
> + gpio->base + ASPEED_SGPIO_CTRL);
> +
> + spin_lock_init(&gpio->lock);
> +
> + gpio->chip.parent = &pdev->dev;
> + gpio->chip.ngpio = nr_gpios;
> + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> + gpio->chip.request = NULL;
> + gpio->chip.free = NULL;
> + gpio->chip.get = aspeed_sgpio_get;
> + gpio->chip.set = aspeed_sgpio_set;
> + gpio->chip.set_config = NULL;
> + gpio->chip.label = dev_name(&pdev->dev);
> + gpio->chip.base = ARCH_NR_GPIOS - MAX_NR_SGPIO;
> +
> + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> + if (rc < 0)
> + return rc;
> +
> + return aspeed_sgpio_setup_irqs(gpio, pdev);
> +}
> +
> +static struct platform_driver aspeed_sgpio_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = aspeed_sgpio_of_table,
> + },
> +};
> +
> +module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
> +MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
> >
>
> thanks Anrew, please review v3 at above and also inline comments at below.
>
> >
> > From: Andrew Jeffery <andrew@aj.id.au>
> > Sent: Wednesday, July 10, 2019 9:46 PM
> > To: Hongwei Zhang; Bartosz Golaszewski; Joel Stanley; Linus Walleij
> > Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org;
> > linux-kernel at vger.kernel.org
> > Subject: Re: [PATCH 2/3 v2] ARM: dts: aspeed: Add SGPIO driver
> >
> >
> >
> > On Thu, 11 Jul 2019, at 00:56, Hongwei Zhang wrote:
> > > Add SGPIO driver support for Aspeed AST2500 SoC.
> > >
> > > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > > ---
> > > drivers/gpio/sgpio-aspeed.c | 450
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 450 insertions(+)
> > > create mode 100644 drivers/gpio/sgpio-aspeed.c
> > >
> > > diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> > > new file mode 100644 index 0000000..0743d22
> > > --- /dev/null
> > > +++ b/drivers/gpio/sgpio-aspeed.c
> > > @@ -0,0 +1,450 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2019 American Megatrends International LLC.
> > > + *
> > > + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in> */
> > > +
> > > +#include <linux/gpio/driver.h>
> > > +#include <linux/gpio/aspeed.h>
> > > +#include <linux/hashtable.h>
> > > +#include <linux/init.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/pinctrl/consumer.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/string.h>
> > > +
> > > +#define NR_SGPIO 80
> > > +
> > > +struct aspeed_sgpio {
> > > + struct gpio_chip chip;
> > > + spinlock_t lock;
> > > + void __iomem *base;
> > > + int irq;
> > > +};
> > > +
> > > +struct aspeed_sgpio_bank {
> > > + uint16_t val_regs;
> > > + uint16_t rdata_reg;
> > > + uint16_t irq_regs;
> > > + const char names[4][3];
> > > +};
> > > +
> > > +/*
> > > + * Note: The "value" register returns the input value sampled on the
> > > + * line even when the GPIO is configured as an output. Since
> > > + * that input goes through synchronizers, writing, then reading
> > > + * back may not return the written value right away.
> > > + *
> > > + * The "rdata" register returns the content of the write latch
> > > + * and thus can be used to read back what was last written
> > > + * reliably.
> > > + */
> > > +
> > > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> > > + {
> > > + .val_regs = 0x0000,
> > > + .rdata_reg = 0x0070,
> > > + .irq_regs = 0x0004,
> > > + .names = { "A", "B", "C", "D" },
> > > + },
> > > + {
> > > + .val_regs = 0x001C,
> > > + .rdata_reg = 0x0074,
> > > + .irq_regs = 0x0020,
> > > + .names = { "E", "F", "G", "H" },
> > > + },
> > > + {
> > > + .val_regs = 0x0038,
> > > + .rdata_reg = 0x0078,
> > > + .irq_regs = 0x003C,
> > > + .names = { "I", "J" },
> > > + },
> > > +};
> > > +
> > > +enum aspeed_sgpio_reg {
> > > + reg_val,
> > > + reg_rdata,
> > > + reg_irq_enable,
> > > + reg_irq_type0,
> > > + reg_irq_type1,
> > > + reg_irq_type2,
> > > + reg_irq_status,
> > > +};
> > > +
> > > +#define GPIO_VAL_VALUE 0x00
> > > +#define GPIO_VAL_DIR 0x04
> > > +#define GPIO_IRQ_ENABLE 0x00
> > > +#define GPIO_IRQ_TYPE0 0x04
> > > +#define GPIO_IRQ_TYPE1 0x08
> > > +#define GPIO_IRQ_TYPE2 0x0C
> > > +#define GPIO_IRQ_STATUS 0x10
> > > +
> > > +/* This will be resolved at compile time */ static inline void
> > > +__iomem *bank_reg(struct aspeed_sgpio *gpio,
> > > + const struct aspeed_sgpio_bank *bank,
> > > + const enum aspeed_sgpio_reg reg) {
> > > + switch (reg) {
> > > + case reg_val:
> > > + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> > > + case reg_rdata:
> > > + return gpio->base + bank->rdata_reg;
> > > + case reg_irq_enable:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> > > + case reg_irq_type0:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> > > + case reg_irq_type1:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> > > + case reg_irq_type2:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> > > + case reg_irq_status:
> > > + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> > > + default:
> > > + /* acturally if code runs to here, it's an error case */
> > > + WARN_ON(reg);
> > > + return gpio->base;
> > > + }
> > > +}
> > > +
>
> updated to use BUG_ON(1), please see v3.
>
> > > +#define GPIO_BANK(x) ((x) >> 5)
> > > +#define GPIO_OFFSET(x) ((x) & 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);
> > > +
> > > + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> > > + return &aspeed_sgpio_banks[bank];
> > > +}
> > > +
> > > +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);
> > > +
> > > + return !!(ioread32(bank_reg(gpio, bank, reg_val)) &
> > > +GPIO_BIT(offset)); }
> > > +
> > > +static void aspeed_sgpio_set(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);
> > > + unsigned long flags;
> > > + void __iomem *addr;
> > > + u32 reg = 0;
> > > +
> > > + spin_lock_irqsave(&gpio->lock, flags);
> > > +
> > > + addr = bank_reg(gpio, bank, reg_val);
> > > +
> > > + if (val)
> > > + reg |= GPIO_BIT(offset);
> > > + else
> > > + reg &= ~GPIO_BIT(offset);
> > > +
> > > + iowrite32(reg, addr);
> > > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > > +
> > > +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int
> > > offset)
> > > +{
> > > + /* By default all SGPIO Pins are input */
> > > + return 0;
> > > +}
> > > +
> > > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> > > offset, int val)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> > > int offset)
> > > +{
> > > + /* By default all SGPIO Pins are input */
> > > + return 1;
> > > +
> > > +}
> >
> > Please see my follow-up reply on v1 that helps clarify what we should do with the
> > dir_in()/dir_out()/get_direction() implementations. The implementation here will confuse everything in
> > the stack above it.
> >
>
> updated, please see v3.
>
> > > +
> > > +static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
> > > + struct aspeed_sgpio **gpio,
> > > + const struct aspeed_sgpio_bank **bank,
> > > + u32 *bit, int *offset)
> > > +{
> > > + struct aspeed_sgpio *internal;
> > > +
> > > + *offset = irqd_to_hwirq(d);
> > > +
> > > + internal = irq_data_get_irq_chip_data(d);
> > > +
> > > + *gpio = internal;
> > > + *bank = to_bank(*offset);
> > > + *bit = GPIO_BIT(*offset);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void aspeed_sgpio_irq_ack(struct irq_data *d) {
> > > + const struct aspeed_sgpio_bank *bank;
> > > + struct aspeed_sgpio *gpio;
> > > + unsigned long flags;
> > > + void __iomem *status_addr;
> > > + int rc, offset;
> > > + u32 bit;
> > > +
> > > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > > + if (rc)
> > > + return;
> > > +
> > > + status_addr = bank_reg(gpio, bank, reg_irq_status);
> > > +
> > > + spin_lock_irqsave(&gpio->lock, flags);
> > > +
> > > + iowrite32(bit, status_addr);
> > > +
> > > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > > +
> > > +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) {
> > > + const struct aspeed_sgpio_bank *bank;
> > > + struct aspeed_sgpio *gpio;
> > > + unsigned long flags;
> > > + u32 reg, bit;
> > > + void __iomem *addr;
> > > + int rc, offset;
> > > +
> > > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > > + if (rc)
> > > + return;
> > > +
> > > + addr = bank_reg(gpio, bank, reg_irq_enable);
> > > +
> > > + spin_lock_irqsave(&gpio->lock, flags);
> > > +
> > > + reg = ioread32(addr);
> > > + if (set)
> > > + reg |= bit;
> > > + else
> > > + reg &= ~bit;
> > > +
> > > + iowrite32(reg, addr);
> > > +
> > > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > > +
> > > +static void aspeed_sgpio_irq_mask(struct irq_data *d) {
> > > + aspeed_sgpio_irq_set_mask(d, false); }
> > > +
> > > +static void aspeed_sgpio_irq_unmask(struct irq_data *d) {
> > > + aspeed_sgpio_irq_set_mask(d, true);
> > > +}
> > > +
> > > +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int
> > > +type) {
> > > + u32 type0 = 0;
> > > + u32 type1 = 0;
> > > + u32 type2 = 0;
> > > + u32 bit, reg;
> > > + const struct aspeed_sgpio_bank *bank;
> > > + irq_flow_handler_t handler;
> > > + struct aspeed_sgpio *gpio;
> > > + unsigned long flags;
> > > + void __iomem *addr;
> > > + int rc, offset;
> > > +
> > > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > > + if (rc)
> > > + return -EINVAL;
> > > +
> > > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > > + case IRQ_TYPE_EDGE_BOTH:
> > > + type2 |= bit;
> > > + /* fall through */
> > > + case IRQ_TYPE_EDGE_RISING:
> > > + type0 |= bit;
> > > + /* fall through */
> > > + case IRQ_TYPE_EDGE_FALLING:
> > > + handler = handle_edge_irq;
> > > + break;
> > > + case IRQ_TYPE_LEVEL_HIGH:
> > > + type0 |= bit;
> > > + /* fall through */
> > > + case IRQ_TYPE_LEVEL_LOW:
> > > + type1 |= bit;
> > > + handler = handle_level_irq;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + spin_lock_irqsave(&gpio->lock, flags);
> > > +
> > > + addr = bank_reg(gpio, bank, reg_irq_type0);
> > > + reg = ioread32(addr);
> > > + reg = (reg & ~bit) | type0;
> > > + iowrite32(reg, addr);
> > > +
> > > + addr = bank_reg(gpio, bank, reg_irq_type1);
> > > + reg = ioread32(addr);
> > > + reg = (reg & ~bit) | type1;
> > > + iowrite32(reg, addr);
> > > +
> > > + addr = bank_reg(gpio, bank, reg_irq_type2);
> > > + reg = ioread32(addr);
> > > + reg = (reg & ~bit) | type2;
> > > + iowrite32(reg, addr);
> > > +
> > > + spin_unlock_irqrestore(&gpio->lock, flags);
> > > +
> > > + irq_set_handler_locked(d, handler);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void aspeed_sgpio_irq_handler(struct irq_desc *desc) {
> > > + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > > + struct irq_chip *ic = irq_desc_get_chip(desc);
> > > + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> > > + unsigned int i, p, girq;
> > > + unsigned long reg;
> > > +
> > > + chained_irq_enter(ic, desc);
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > > + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> > > +
> > > + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> > > +
> > > + for_each_set_bit(p, ®, 32) {
> > > + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> > > + generic_handle_irq(girq);
> > > + }
> > > +
> > > + }
> > > +
> > > + chained_irq_exit(ic, desc);
> > > +}
> > > +
> > > +static struct irq_chip aspeed_sgpio_irqchip = {
> > > + .name = "aspeed-sgpio",
> > > + .irq_ack = aspeed_sgpio_irq_ack,
> > > + .irq_mask = aspeed_sgpio_irq_mask,
> > > + .irq_unmask = aspeed_sgpio_irq_unmask,
> > > + .irq_set_type = aspeed_sgpio_set_type,
> > > +};
> > > +
> > > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> > > + struct platform_device *pdev)
> > > +{
> > > + int rc, i;
> > > + const struct aspeed_sgpio_bank *bank;
> > > +
> > > + rc = platform_get_irq(pdev, 0);
> > > + if (rc < 0)
> > > + return rc;
> > > +
> > > + gpio->irq = rc;
> > > +
> > > + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> > > Pins. */
> > > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > > + bank = &aspeed_sgpio_banks[i];
> > > + /* disable irq enable bits */
> > > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> > > + /* clear status bits */
> > > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> > > + }
> > > +
> > > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> > > + 0, handle_bad_irq, IRQ_TYPE_NONE);
> > > + if (rc) {
> > > + dev_info(&pdev->dev, "Could not add irqchip\n");
> > > + return rc;
> > > + }
> > > +
> > > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> > > + gpio->irq, aspeed_sgpio_irq_handler);
> > > +
> > > + /* set IRQ settings and Enable Interrupt */
> > > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > > + bank = &aspeed_sgpio_banks[i];
> > > + /* set falling or level-low irq */
> > > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> > > + /* trigger type is edge */
> > > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> > > + /* dual edge trigger mode. */
> > > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> > > + /* enable irq */
> > > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int aspeed_sgpio_request(struct gpio_chip *chip, unsigned int
> > > offset)
> > > +{
> > > + return (offset < NR_SGPIO);
> > > +}
> >
> > I don't think this request() implementation is helpful, especially as it stands in the face of needing to pull
> > the number of GPIOs to serialise from the devicetree.
> >
> > request() is an optional callback, lets just drop it.
> >
>
> updated, please see v3.
>
> > > +
> > > +static const struct of_device_id aspeed_sgpio_of_table[] = {
> > > + { .compatible = "aspeed,ast2400-sgpio" },
> > > + { .compatible = "aspeed,ast2500-sgpio" },
> > > + {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> > > +
> > > +static int __init aspeed_sgpio_probe(struct platform_device *pdev) {
> > > + struct aspeed_sgpio *gpio;
> > > + struct resource *res;
> > > + int rc;
> > > +
> > > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> > > + if (!gpio)
> > > + return -ENOMEM;
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + gpio->base = devm_ioremap_resource(&pdev->dev, res);
> > > + if (IS_ERR(gpio->base))
> > > + return PTR_ERR(gpio->base);
> > > +
> > > + spin_lock_init(&gpio->lock);
> > > +
> > > + gpio->chip.parent = &pdev->dev;
> > > + gpio->chip.ngpio = NR_SGPIO;
> > > + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> > > + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> > > + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> > > + gpio->chip.request = aspeed_sgpio_request;
> > > + gpio->chip.free = NULL;
> > > + gpio->chip.get = aspeed_sgpio_get;
> > > + gpio->chip.set = aspeed_sgpio_set;
> > > + gpio->chip.set_config = NULL;
> > > + gpio->chip.label = dev_name(&pdev->dev);
> > > + gpio->chip.base = -1;
> > > +
> > > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> > > + if (rc < 0)
> > > + return rc;
> > > +
> > > + return aspeed_sgpio_setup_irqs(gpio, pdev); }
> > > +
> > > +static struct platform_driver aspeed_sgpio_driver = {
> > > + .driver = {
> > > + .name = KBUILD_MODNAME,
> > > + .of_match_table = aspeed_sgpio_of_table,
> > > + },
> > > +};
> > > +
> > > +module_platform_driver_probe(aspeed_sgpio_driver,
> > > +aspeed_sgpio_probe); MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.7.4
> > >
> > >
>
^ permalink raw reply
* [PATCH 2/3 v3] dt-bindings: gpio: aspeed: Add SGPIO support
From: Hongwei Zhang @ 2019-07-16 21:48 UTC (permalink / raw)
To: linux-aspeed
Add bindings to support SGPIO on AST2400 or AST2500.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
.../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
new file mode 100644
index 0000000..8c3a747
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
@@ -0,0 +1,55 @@
+Aspeed SGPIO controller Device Tree Bindings
+-------------------------------------------
+
+This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80 full
+featured Serial GPIOs. Each of the Serial GPIO pins can be programmed to
+support the following options:
+- Support interrupt option for each input port and various interrupt
+ sensitivity option (level-high, level-low, edge-high, edge-low)
+- Support reset tolerance option for each output port
+- Directly connected to APB bus and its shift clock is from APB bus clock
+ divided by a programmable value.
+- Co-work with external signal-chained TTL components (74LV165/74LV595)
+
+
+Required properties:
+
+- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
+
+- #gpio-cells : Should be two
+ - First cell is the GPIO line number
+ - Second cell is used to specify optional
+ parameters (unused)
+
+- reg : Address and length of the register set for the device
+- gpio-controller : Marks the device node as a GPIO controller.
+- interrupts : Interrupt specifier (see interrupt bindings for
+ details)
+
+- interrupt-controller : Mark the GPIO controller as an interrupt-controller
+
+- nr-gpios : number of GPIO pins to serialise.
+ (should be multiple of 8, up to 80 pins; 0 if not used)
+
+- clocks : A phandle to the APB clock for SGPM clock division
+
+- bus-frequency : SGPM CLK frequency, derived from APB bus clock by a programmable devisor
+
+
+The sgpio and interrupt properties are further described in their respective bindings documentation:
+
+- Documentation/devicetree/bindings/sgpio/gpio.txt
+- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+
+ Example:
+ sgpio at 1e780200 {
+ #gpio-cells = <2>;
+ compatible = "aspeed,ast2500-sgpio";
+ gpio-controller;
+ interrupts = <40>;
+ reg = <0x1e780200 0x0100>;
+ clocks = <&syscon ASPEED_CLK_APB>;
+ interrupt-controller;
+ nr-gpios = <8>;
+ bus-frequency = <12000000>;
+ };
--
2.7.4
Thanks Andrew, please see above v3 and inline comments at below.
--Hongwei
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Sunday, July 14, 2019 10:25 PM
> To: Hongwei Zhang; Joel Stanley; Linus Walleij; devicetree at vger.kernel.org
> Cc: Rob Herring; Mark Rutland; Bartosz Golaszewski; linux-aspeed at lists.ozlabs.org; linux-
> kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-gpio at vger.kernel.org
> Subject: Re: [PATCH 2/3 v2] dt-bindings: gpio: aspeed: Add SGPIO support
>
> Hello Hongwei,
>
> On Sat, 13 Jul 2019, at 05:44, Hongwei Zhang wrote:
> > Add bindings to support SGPIO on AST2400 or AST2500.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> > .../devicetree/bindings/gpio/sgpio-aspeed.txt | 43 ++++++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> > create mode 100755
> > Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > new file mode 100755
> > index 0000000..3ae2b79
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > @@ -0,0 +1,43 @@
> > +Aspeed SGPIO controller Device Tree Bindings
> > +-------------------------------------------
> > +
> > +Required properties:
> > +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> > +
> > +- #gpio-cells : Should be two
> > + - First cell is the GPIO line number
> > + - Second cell is used to specify optional
> > + parameters (unused)
> > +
> > +- reg : Address and length of the register set for the device
> > +- gpio-controller : Marks the device node as a GPIO controller.
> > +- interrupts : Interrupt specifier (see interrupt bindings for
> > + details)
> > +
> > +- interrupt-controller : Mark the GPIO controller as an
> > interrupt-controller
> > +
> > +- nr-gpios : number of GPIO pins to serialise. (should be multiple of
> > 8, up to 80 pins)
> > + if not specified, defaults to 80.
>
> This appears to be a statement about the driver implementation, but bindings documents are about
> describing hardware. Reading the datasheet it actually appears the ASPEED SGPIO hardware comes up
> in what is "technically" a forbidden state (equivalent to `nr-gpios = <0>;`), though the device is also
> disabled at this point, so it's probably moot. The point is the true default value from a hardware
> perspective is 0, not 80, so if we're going to talk about default values, 0 would be more appropriate.
> However:
>
> You've also listed nr-gpios under the "Required properties" header, but the description suggests it's
> optional. It's either one or the other, please lets be clear about it. On that front, lets make it nr-gpios
> *not* optional (i.e. make it
> required) thus force the specification of how many SGPIOs we want to emit on the bus. This value is
> coupled to the platform design, so I don't think there's ever a scenario where we want nr-gpios to take a
> default value.
>
Added some descriptions and updated nr-gpios, please see v3.
> > +
> > +- clocks : A phandle to the APB clock for SGPM clock
> > division
> > +
> > +- bus-frequency : SGPM CLK frequency, if not specified defaults to 1
> > MHz
>
> Again here with the default value - SGPM CLK period is derived from PCLK by the expression `period =
> PCLK * 2 *(GPIO254[31:16] + 1)`, where GPIO254's initialisation state is `GPIO254[31:16] = 0`, which
> gives a default SGPM bus frequency of PCLK / 2. This is likely not going to be 1MHz (more like ~12MHz).
>
> Lets just make the property required. That way we avoid any ambiguity about the bus frequency and
> thus don't need words about defaults that turn out to be about the driver, not about the hardware.
>
updated, please see v3.
> Finally, when updating patches in response to feedback, please send the full series again, and bump the
> series version number. That way people can review a coherent set of patches and not have to hunt
> around and (fail to) collate the correct combination. It makes it easier to say "Reviewed-by:" on your
> patches :)
>
> Cheers,
>
> Andrew
^ permalink raw reply related
* [PATCH 2/3 v3] ARM: dts: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-16 21:24 UTC (permalink / raw)
To: linux-aspeed
Add SGPIO driver support for Aspeed AST2500 SoC.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
drivers/gpio/sgpio-aspeed.c | 487 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 487 insertions(+)
create mode 100644 drivers/gpio/sgpio-aspeed.c
diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
new file mode 100644
index 0000000..ade2cb7
--- /dev/null
+++ b/drivers/gpio/sgpio-aspeed.c
@@ -0,0 +1,487 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 American Megatrends International LLC.
+ *
+ * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/gpio/aspeed.h>
+#include <linux/hashtable.h>
+#include <linux/bitfield.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/gpio.h>
+
+#define MAX_NR_SGPIO 80
+
+#define ASPEED_SGPIO_CTRL 0x54
+
+#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
+#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
+#define ASPEED_SGPIO_ENABLE BIT(0)
+
+// default sgpio direction is input.
+static uint32_t sgpio_dir_val[3] = {0xffffffff, 0xffffffff, 0xffffffff };
+
+struct aspeed_sgpio {
+ struct gpio_chip chip;
+ struct clk *pclk;
+ spinlock_t lock;
+ void __iomem *base;
+ int irq;
+};
+
+struct aspeed_sgpio_bank {
+ uint16_t val_regs;
+ uint16_t rdata_reg;
+ uint16_t irq_regs;
+ const char names[4][3];
+};
+
+/*
+ * Note: The "value" register returns the input value sampled on the
+ * line even when the GPIO is configured as an output. Since
+ * that input goes through synchronizers, writing, then reading
+ * back may not return the written value right away.
+ *
+ * The "rdata" register returns the content of the write latch
+ * and thus can be used to read back what was last written
+ * reliably.
+ */
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
+
+enum aspeed_sgpio_reg {
+ reg_val,
+ reg_rdata,
+ reg_irq_enable,
+ reg_irq_type0,
+ reg_irq_type1,
+ reg_irq_type2,
+ reg_irq_status,
+};
+
+#define GPIO_VAL_VALUE 0x00
+#define GPIO_VAL_DIR 0x04
+#define GPIO_IRQ_ENABLE 0x00
+#define GPIO_IRQ_TYPE0 0x04
+#define GPIO_IRQ_TYPE1 0x08
+#define GPIO_IRQ_TYPE2 0x0C
+#define GPIO_IRQ_STATUS 0x10
+
+/* This will be resolved at compile time */
+static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
+ const struct aspeed_sgpio_bank *bank,
+ const enum aspeed_sgpio_reg reg)
+{
+ switch (reg) {
+ case reg_val:
+ return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
+ case reg_rdata:
+ return gpio->base + bank->rdata_reg;
+ case reg_irq_enable:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
+ case reg_irq_type0:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
+ case reg_irq_type1:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
+ case reg_irq_type2:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
+ case reg_irq_status:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
+ default:
+ /* acturally if code runs to here, it's an error case */
+ BUG_ON(1);
+ }
+}
+
+#define GPIO_BANK(x) ((x) >> 5)
+#define GPIO_OFFSET(x) ((x) & 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);
+
+ WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
+ return &aspeed_sgpio_banks[bank];
+}
+
+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);
+
+ if (sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset))
+ return !!(ioread32(bank_reg(gpio, bank, reg_val)) & GPIO_BIT(offset));
+ else
+ return !!(ioread32(bank_reg(gpio, bank, reg_rdata)) & GPIO_BIT(offset));
+
+}
+
+static void aspeed_sgpio_set(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);
+ unsigned long flags;
+ void __iomem *addr;
+ u32 reg = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_val);
+
+ if (val)
+ reg |= GPIO_BIT(offset);
+ else
+ reg &= ~GPIO_BIT(offset);
+
+ iowrite32(reg, addr);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+ sgpio_dir_val[GPIO_BANK(offset)] |= GPIO_BIT(offset);
+ return 0;
+}
+
+static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ sgpio_dir_val[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
+ return 0;
+}
+
+static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ return sgpio_dir_val[GPIO_BANK(offset)] & GPIO_BIT(offset);
+
+}
+
+static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
+ struct aspeed_sgpio **gpio,
+ const struct aspeed_sgpio_bank **bank,
+ u32 *bit, int *offset)
+{
+ struct aspeed_sgpio *internal;
+
+ *offset = irqd_to_hwirq(d);
+
+ internal = irq_data_get_irq_chip_data(d);
+
+ *gpio = internal;
+ *bank = to_bank(*offset);
+ *bit = GPIO_BIT(*offset);
+
+ return 0;
+}
+
+static void aspeed_sgpio_irq_ack(struct irq_data *d)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *status_addr;
+ int rc, offset;
+ u32 bit;
+
+ rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ if (rc)
+ return;
+
+ status_addr = bank_reg(gpio, bank, reg_irq_status);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ iowrite32(bit, status_addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ u32 reg, bit;
+ void __iomem *addr;
+ int rc, offset;
+
+ rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ if (rc)
+ return;
+
+ addr = bank_reg(gpio, bank, reg_irq_enable);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ reg = ioread32(addr);
+ if (set)
+ reg |= bit;
+ else
+ reg &= ~bit;
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_mask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, false);
+}
+
+static void aspeed_sgpio_irq_unmask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, true);
+}
+
+static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
+{
+ u32 type0 = 0;
+ u32 type1 = 0;
+ u32 type2 = 0;
+ u32 bit, reg;
+ const struct aspeed_sgpio_bank *bank;
+ irq_flow_handler_t handler;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *addr;
+ int rc, offset;
+
+ rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ if (rc)
+ return -EINVAL;
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_BOTH:
+ type2 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_RISING:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_FALLING:
+ handler = handle_edge_irq;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_LEVEL_LOW:
+ type1 |= bit;
+ handler = handle_level_irq;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_irq_type0);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type0;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type1);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type1;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type2);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type2;
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ irq_set_handler_locked(d, handler);
+
+ return 0;
+}
+
+static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *ic = irq_desc_get_chip(desc);
+ struct aspeed_sgpio *data = gpiochip_get_data(gc);
+ unsigned int i, p, girq;
+ unsigned long reg;
+
+ chained_irq_enter(ic, desc);
+
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
+
+ reg = ioread32(bank_reg(data, bank, reg_irq_status));
+
+ for_each_set_bit(p, ®, 32) {
+ girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
+ generic_handle_irq(girq);
+ }
+
+ }
+
+ chained_irq_exit(ic, desc);
+}
+
+static struct irq_chip aspeed_sgpio_irqchip = {
+ .name = "aspeed-sgpio",
+ .irq_ack = aspeed_sgpio_irq_ack,
+ .irq_mask = aspeed_sgpio_irq_mask,
+ .irq_unmask = aspeed_sgpio_irq_unmask,
+ .irq_set_type = aspeed_sgpio_set_type,
+};
+
+static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
+ struct platform_device *pdev)
+{
+ int rc, i;
+ const struct aspeed_sgpio_bank *bank;
+
+ rc = platform_get_irq(pdev, 0);
+ if (rc < 0)
+ return rc;
+
+ gpio->irq = rc;
+
+ /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* disable irq enable bits */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
+ /* clear status bits */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
+ }
+
+ rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
+ 0, handle_bad_irq, IRQ_TYPE_NONE);
+ if (rc) {
+ dev_info(&pdev->dev, "Could not add irqchip\n");
+ return rc;
+ }
+
+ gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
+ gpio->irq, aspeed_sgpio_irq_handler);
+
+ /* set IRQ settings and Enable Interrupt */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* set falling or level-low irq */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
+ /* trigger type is edge */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
+ /* dual edge trigger mode. */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
+ /* enable irq */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
+ }
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_sgpio_of_table[] = {
+ { .compatible = "aspeed,ast2400-sgpio" },
+ { .compatible = "aspeed,ast2500-sgpio" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
+
+static int __init aspeed_sgpio_probe(struct platform_device *pdev)
+{
+ struct aspeed_sgpio *gpio;
+ struct resource *res;
+ u32 nr_gpios, sgpio_freq;
+ int rc;
+ u16 sgpio_clk_div;
+ unsigned long apb_freq;
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ gpio->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(gpio->base))
+ return PTR_ERR(gpio->base);
+
+ rc = of_property_read_u32(pdev->dev.of_node, "nr-gpios", &nr_gpios);
+ if ((rc < 0) || (nr_gpios > MAX_NR_SGPIO))
+ nr_gpios = MAX_NR_SGPIO;
+
+ rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+ sgpio_freq = 12000000;
+ }
+
+ gpio->pclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(gpio->pclk)) {
+ dev_err(&pdev->dev, "devm_clk_get failed\n");
+ return PTR_ERR(gpio->pclk);
+ }
+
+ apb_freq = clk_get_rate(gpio->pclk);
+ sgpio_clk_div = 2 * ((apb_freq % sgpio_freq == 0) ?
+ (apb_freq / sgpio_freq) - 1 : (apb_freq / sgpio_freq));
+ iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
+ FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
+ ASPEED_SGPIO_ENABLE,
+ gpio->base + ASPEED_SGPIO_CTRL);
+
+ spin_lock_init(&gpio->lock);
+
+ gpio->chip.parent = &pdev->dev;
+ gpio->chip.ngpio = nr_gpios;
+ gpio->chip.direction_input = aspeed_sgpio_dir_in;
+ gpio->chip.direction_output = aspeed_sgpio_dir_out;
+ gpio->chip.get_direction = aspeed_sgpio_get_direction;
+ gpio->chip.request = NULL;
+ gpio->chip.free = NULL;
+ gpio->chip.get = aspeed_sgpio_get;
+ gpio->chip.set = aspeed_sgpio_set;
+ gpio->chip.set_config = NULL;
+ gpio->chip.label = dev_name(&pdev->dev);
+ gpio->chip.base = ARCH_NR_GPIOS - MAX_NR_SGPIO;
+
+ rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+ if (rc < 0)
+ return rc;
+
+ return aspeed_sgpio_setup_irqs(gpio, pdev);
+}
+
+static struct platform_driver aspeed_sgpio_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = aspeed_sgpio_of_table,
+ },
+};
+
+module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
+MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
>
thanks Anrew, please review v3 at above and also inline comments at below.
>
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Wednesday, July 10, 2019 9:46 PM
> To: Hongwei Zhang; Bartosz Golaszewski; Joel Stanley; Linus Walleij
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org;
> linux-kernel at vger.kernel.org
> Subject: Re: [PATCH 2/3 v2] ARM: dts: aspeed: Add SGPIO driver
>
>
>
> On Thu, 11 Jul 2019, at 00:56, Hongwei Zhang wrote:
> > Add SGPIO driver support for Aspeed AST2500 SoC.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> > drivers/gpio/sgpio-aspeed.c | 450
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 450 insertions(+)
> > create mode 100644 drivers/gpio/sgpio-aspeed.c
> >
> > diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> > new file mode 100644 index 0000000..0743d22
> > --- /dev/null
> > +++ b/drivers/gpio/sgpio-aspeed.c
> > @@ -0,0 +1,450 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019 American Megatrends International LLC.
> > + *
> > + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in> */
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/gpio/aspeed.h>
> > +#include <linux/hashtable.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +
> > +#define NR_SGPIO 80
> > +
> > +struct aspeed_sgpio {
> > + struct gpio_chip chip;
> > + spinlock_t lock;
> > + void __iomem *base;
> > + int irq;
> > +};
> > +
> > +struct aspeed_sgpio_bank {
> > + uint16_t val_regs;
> > + uint16_t rdata_reg;
> > + uint16_t irq_regs;
> > + const char names[4][3];
> > +};
> > +
> > +/*
> > + * Note: The "value" register returns the input value sampled on the
> > + * line even when the GPIO is configured as an output. Since
> > + * that input goes through synchronizers, writing, then reading
> > + * back may not return the written value right away.
> > + *
> > + * The "rdata" register returns the content of the write latch
> > + * and thus can be used to read back what was last written
> > + * reliably.
> > + */
> > +
> > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> > + {
> > + .val_regs = 0x0000,
> > + .rdata_reg = 0x0070,
> > + .irq_regs = 0x0004,
> > + .names = { "A", "B", "C", "D" },
> > + },
> > + {
> > + .val_regs = 0x001C,
> > + .rdata_reg = 0x0074,
> > + .irq_regs = 0x0020,
> > + .names = { "E", "F", "G", "H" },
> > + },
> > + {
> > + .val_regs = 0x0038,
> > + .rdata_reg = 0x0078,
> > + .irq_regs = 0x003C,
> > + .names = { "I", "J" },
> > + },
> > +};
> > +
> > +enum aspeed_sgpio_reg {
> > + reg_val,
> > + reg_rdata,
> > + reg_irq_enable,
> > + reg_irq_type0,
> > + reg_irq_type1,
> > + reg_irq_type2,
> > + reg_irq_status,
> > +};
> > +
> > +#define GPIO_VAL_VALUE 0x00
> > +#define GPIO_VAL_DIR 0x04
> > +#define GPIO_IRQ_ENABLE 0x00
> > +#define GPIO_IRQ_TYPE0 0x04
> > +#define GPIO_IRQ_TYPE1 0x08
> > +#define GPIO_IRQ_TYPE2 0x0C
> > +#define GPIO_IRQ_STATUS 0x10
> > +
> > +/* This will be resolved at compile time */ static inline void
> > +__iomem *bank_reg(struct aspeed_sgpio *gpio,
> > + const struct aspeed_sgpio_bank *bank,
> > + const enum aspeed_sgpio_reg reg) {
> > + switch (reg) {
> > + case reg_val:
> > + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> > + case reg_rdata:
> > + return gpio->base + bank->rdata_reg;
> > + case reg_irq_enable:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> > + case reg_irq_type0:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> > + case reg_irq_type1:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> > + case reg_irq_type2:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> > + case reg_irq_status:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> > + default:
> > + /* acturally if code runs to here, it's an error case */
> > + WARN_ON(reg);
> > + return gpio->base;
> > + }
> > +}
> > +
updated to use BUG_ON(1), please see v3.
> > +#define GPIO_BANK(x) ((x) >> 5)
> > +#define GPIO_OFFSET(x) ((x) & 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);
> > +
> > + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> > + return &aspeed_sgpio_banks[bank];
> > +}
> > +
> > +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);
> > +
> > + return !!(ioread32(bank_reg(gpio, bank, reg_val)) &
> > +GPIO_BIT(offset)); }
> > +
> > +static void aspeed_sgpio_set(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);
> > + unsigned long flags;
> > + void __iomem *addr;
> > + u32 reg = 0;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + addr = bank_reg(gpio, bank, reg_val);
> > +
> > + if (val)
> > + reg |= GPIO_BIT(offset);
> > + else
> > + reg &= ~GPIO_BIT(offset);
> > +
> > + iowrite32(reg, addr);
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int
> > offset)
> > +{
> > + /* By default all SGPIO Pins are input */
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> > offset, int val)
> > +{
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> > int offset)
> > +{
> > + /* By default all SGPIO Pins are input */
> > + return 1;
> > +
> > +}
>
> Please see my follow-up reply on v1 that helps clarify what we should do with the
> dir_in()/dir_out()/get_direction() implementations. The implementation here will confuse everything in
> the stack above it.
>
updated, please see v3.
> > +
> > +static inline int irqd_to_aspeed_sgpio_data(struct irq_data *d,
> > + struct aspeed_sgpio **gpio,
> > + const struct aspeed_sgpio_bank **bank,
> > + u32 *bit, int *offset)
> > +{
> > + struct aspeed_sgpio *internal;
> > +
> > + *offset = irqd_to_hwirq(d);
> > +
> > + internal = irq_data_get_irq_chip_data(d);
> > +
> > + *gpio = internal;
> > + *bank = to_bank(*offset);
> > + *bit = GPIO_BIT(*offset);
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_sgpio_irq_ack(struct irq_data *d) {
> > + const struct aspeed_sgpio_bank *bank;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + void __iomem *status_addr;
> > + int rc, offset;
> > + u32 bit;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return;
> > +
> > + status_addr = bank_reg(gpio, bank, reg_irq_status);
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + iowrite32(bit, status_addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) {
> > + const struct aspeed_sgpio_bank *bank;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + u32 reg, bit;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return;
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_enable);
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + reg = ioread32(addr);
> > + if (set)
> > + reg |= bit;
> > + else
> > + reg &= ~bit;
> > +
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static void aspeed_sgpio_irq_mask(struct irq_data *d) {
> > + aspeed_sgpio_irq_set_mask(d, false); }
> > +
> > +static void aspeed_sgpio_irq_unmask(struct irq_data *d) {
> > + aspeed_sgpio_irq_set_mask(d, true);
> > +}
> > +
> > +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int
> > +type) {
> > + u32 type0 = 0;
> > + u32 type1 = 0;
> > + u32 type2 = 0;
> > + u32 bit, reg;
> > + const struct aspeed_sgpio_bank *bank;
> > + irq_flow_handler_t handler;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + void __iomem *addr;
> > + int rc, offset;
> > +
> > + rc = irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + if (rc)
> > + return -EINVAL;
> > +
> > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_EDGE_BOTH:
> > + type2 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_EDGE_RISING:
> > + type0 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_EDGE_FALLING:
> > + handler = handle_edge_irq;
> > + break;
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + type0 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_LEVEL_LOW:
> > + type1 |= bit;
> > + handler = handle_level_irq;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type0);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type0;
> > + iowrite32(reg, addr);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type1);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type1;
> > + iowrite32(reg, addr);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type2);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type2;
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + irq_set_handler_locked(d, handler);
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_sgpio_irq_handler(struct irq_desc *desc) {
> > + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *ic = irq_desc_get_chip(desc);
> > + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> > + unsigned int i, p, girq;
> > + unsigned long reg;
> > +
> > + chained_irq_enter(ic, desc);
> > +
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> > +
> > + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> > +
> > + for_each_set_bit(p, ®, 32) {
> > + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> > + generic_handle_irq(girq);
> > + }
> > +
> > + }
> > +
> > + chained_irq_exit(ic, desc);
> > +}
> > +
> > +static struct irq_chip aspeed_sgpio_irqchip = {
> > + .name = "aspeed-sgpio",
> > + .irq_ack = aspeed_sgpio_irq_ack,
> > + .irq_mask = aspeed_sgpio_irq_mask,
> > + .irq_unmask = aspeed_sgpio_irq_unmask,
> > + .irq_set_type = aspeed_sgpio_set_type,
> > +};
> > +
> > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> > + struct platform_device *pdev)
> > +{
> > + int rc, i;
> > + const struct aspeed_sgpio_bank *bank;
> > +
> > + rc = platform_get_irq(pdev, 0);
> > + if (rc < 0)
> > + return rc;
> > +
> > + gpio->irq = rc;
> > +
> > + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> > Pins. */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + bank = &aspeed_sgpio_banks[i];
> > + /* disable irq enable bits */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> > + /* clear status bits */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> > + }
> > +
> > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> > + 0, handle_bad_irq, IRQ_TYPE_NONE);
> > + if (rc) {
> > + dev_info(&pdev->dev, "Could not add irqchip\n");
> > + return rc;
> > + }
> > +
> > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> > + gpio->irq, aspeed_sgpio_irq_handler);
> > +
> > + /* set IRQ settings and Enable Interrupt */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + bank = &aspeed_sgpio_banks[i];
> > + /* set falling or level-low irq */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> > + /* trigger type is edge */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> > + /* dual edge trigger mode. */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> > + /* enable irq */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_request(struct gpio_chip *chip, unsigned int
> > offset)
> > +{
> > + return (offset < NR_SGPIO);
> > +}
>
> I don't think this request() implementation is helpful, especially as it stands in the face of needing to pull
> the number of GPIOs to serialise from the devicetree.
>
> request() is an optional callback, lets just drop it.
>
updated, please see v3.
> > +
> > +static const struct of_device_id aspeed_sgpio_of_table[] = {
> > + { .compatible = "aspeed,ast2400-sgpio" },
> > + { .compatible = "aspeed,ast2500-sgpio" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> > +
> > +static int __init aspeed_sgpio_probe(struct platform_device *pdev) {
> > + struct aspeed_sgpio *gpio;
> > + struct resource *res;
> > + int rc;
> > +
> > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> > + if (!gpio)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + gpio->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(gpio->base))
> > + return PTR_ERR(gpio->base);
> > +
> > + spin_lock_init(&gpio->lock);
> > +
> > + gpio->chip.parent = &pdev->dev;
> > + gpio->chip.ngpio = NR_SGPIO;
> > + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> > + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> > + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> > + gpio->chip.request = aspeed_sgpio_request;
> > + gpio->chip.free = NULL;
> > + gpio->chip.get = aspeed_sgpio_get;
> > + gpio->chip.set = aspeed_sgpio_set;
> > + gpio->chip.set_config = NULL;
> > + gpio->chip.label = dev_name(&pdev->dev);
> > + gpio->chip.base = -1;
> > +
> > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> > + if (rc < 0)
> > + return rc;
> > +
> > + return aspeed_sgpio_setup_irqs(gpio, pdev); }
> > +
> > +static struct platform_driver aspeed_sgpio_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .of_match_table = aspeed_sgpio_of_table,
> > + },
> > +};
> > +
> > +module_platform_driver_probe(aspeed_sgpio_driver,
> > +aspeed_sgpio_probe); MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.7.4
> >
> >
^ permalink raw reply related
* [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Rob Herring @ 2019-07-16 15:04 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACPK8Xdz98CQzgE2KCjz8eOhPtx=H8jTe1hVT7LvP77U_gGASQ@mail.gmail.com>
On Mon, Jul 15, 2019 at 5:17 PM Joel Stanley <joel@jms.id.au> wrote:
>
> On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
> >
> > The Aspeed pinctl schema have errors in the 'compatible' schema:
> >
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
> >
> > Flow style sequences have to be quoted if the vales contain ','. Fix
> > this by using the more common one line per entry formatting.
>
> >
> > properties:
> > compatible:
> > - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> > + enum:
> > + - aspeed,ast2400-pinctrl
> > + - aspeed,g4-pinctrl
>
> Thanks for the fix. However, we've standardised on the first form for
> all of our device trees, so we can drop the second compatible string
> from the bindings.
Doing that would introduce validation warnings until the dts file is
updated. So we still need this change until that happens.
Rob
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Rob Herring @ 2019-07-16 14:57 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <3fe55ea9-b949-48a0-9eab-90ad3bc1ee2a@www.fastmail.com>
On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > data bus if only a single slot is enabled.
> > >
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > > In v2:
> > >
> > > * Rename to aspeed,sdhci.yaml
> > > * Rename sd-controller compatible
> > > * Add `maxItems: 1` for reg properties
> > > * Move sdhci subnode description to patternProperties
> > > * Drop sdhci compatible requirement
> > > * #address-cells and #size-cells are required
> > > * Prevent additional properties
> > > * Implement explicit ranges in example
> > > * Remove slot property
> > >
> > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > 1 file changed, 90 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > new file mode 100644
> > > index 000000000000..67a691c3348c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > @@ -0,0 +1,90 @@
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ASPEED SD/SDIO/eMMC Controller
> > > +
> > > +maintainers:
> > > + - Andrew Jeffery <andrew@aj.id.au>
> > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > +
> > > +description: |+
> > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > + only a single slot is enabled.
> > > +
> > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > + the slots are dependent on the common configuration area, they are described
> > > + as child nodes.
> > > +
> > > +properties:
> > > + compatible:
> > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> >
> > This is actually a list of 4 strings. Please reformat to 1 per line.
>
> On reflection that's obvious, but also a somewhat subtle interaction with the
> preference for no quotes (the obvious caveat being "except where required").
It wasn't something I'd run into before. I'm working on a check, but
unfortunately we can only check for quotes not needed and can't check
for missing quotes.
> Thanks for pointing it out.
>
> I have been running `make dt_binding_check` and `make dtbs_check` over
> these, looks like I need to up my game a bit though. Do you do additional things
> in your workflow?
That should have thrown the warnings. If you aren't seeing those, do
you have dtschema package installed (see
Documentation/devicetree/writing-schema.md)? Or it could be erroring
out on something else first. There's a few breakages that I'm trying
to fix.
Rob
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Andrew Jeffery @ 2019-07-16 0:39 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACPK8Xdz98CQzgE2KCjz8eOhPtx=H8jTe1hVT7LvP77U_gGASQ@mail.gmail.com>
On Tue, 16 Jul 2019, at 08:47, Joel Stanley wrote:
> On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
> >
> > The Aspeed pinctl schema have errors in the 'compatible' schema:
> >
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> > Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> > properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
> >
> > Flow style sequences have to be quoted if the vales contain ','. Fix
> > this by using the more common one line per entry formatting.
>
> >
> > properties:
> > compatible:
> > - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> > + enum:
> > + - aspeed,ast2400-pinctrl
> > + - aspeed,g4-pinctrl
>
> Thanks for the fix. However, we've standardised on the first form for
> all of our device trees, so we can drop the second compatible string
> from the bindings.
>
> I think Andrew already has a patch cooking to do this.
Yes, I have a series in the works. Will send patches soon.
Andrew
^ permalink raw reply
* Re: [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Andrew Jeffery @ 2019-07-16 0:37 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190715223725.12924-1-robh@kernel.org>
On Tue, 16 Jul 2019, at 08:07, Rob Herring wrote:
> The Aspeed pinctl schema have errors in the 'compatible' schema:
>
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed',
> 'g4-pinctrl'] has non-unique elements
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed',
> 'g5-pinctrl'] has non-unique elements
>
> Flow style sequences have to be quoted if the vales contain ','. Fix
> this by using the more common one line per entry formatting.
>
> Fixes: 0a617de16730 ("dt-bindings: pinctrl: aspeed: Convert AST2500
> bindings to json-schema")
> Fixes: 07457937bb5c ("dt-bindings: pinctrl: aspeed: Convert AST2400
> bindings to json-schema")
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: linux-aspeed at lists.ozlabs.org
> Cc: linux-gpio at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> .../devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml | 4 +++-
> .../devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml | 4 +++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> index 61a110a7db8a..125599a2dc5e 100644
> ---
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> +++
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> @@ -22,7 +22,9 @@ description: |+
>
> properties:
> compatible:
> - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> + enum:
> + - aspeed,ast2400-pinctrl
> + - aspeed,g4-pinctrl
>
> patternProperties:
> '^.*$':
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> index cf561bd55128..a464cfa0cba3 100644
> ---
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> +++
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> @@ -22,7 +22,9 @@ description: |+
>
> properties:
> compatible:
> - enum: [ aspeed,ast2500-pinctrl, aspeed,g5-pinctrl ]
> + enum:
> + - aspeed,ast2500-pinctrl
> + - aspeed,g5-pinctrl
> aspeed,external-nodes:
> minItems: 2
> maxItems: 2
> --
> 2.20.1
>
>
^ permalink raw reply
* [PATCH] dt-bindings: pinctrl: aspeed: Fix AST2500 example errors
From: Andrew Jeffery @ 2019-07-16 0:37 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190715224841.6771-1-robh@kernel.org>
On Tue, 16 Jul 2019, at 08:18, Rob Herring wrote:
> The schema examples are now validated against the schema itself. The
> AST2500 pinctrl schema has a couple of errors:
>
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.example.dt.yaml: \
> example-0: $nodename:0: 'example-0' does not match
> '^(bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.example.dt.yaml: \
> pinctrl: aspeed,external-nodes: [[1, 2]] is too short
>
> Fixes: 0a617de16730 ("dt-bindings: pinctrl: aspeed: Convert AST2500
> bindings to json-schema")
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: linux-aspeed at lists.ozlabs.org
> Cc: linux-gpio at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> .../devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> index a464cfa0cba3..3e6d85318577 100644
> ---
> a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> +++
> b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
> @@ -76,9 +76,6 @@ required:
>
> examples:
> - |
> - compatible = "simple-bus";
> - ranges;
> -
> apb {
> compatible = "simple-bus";
> #address-cells = <1>;
> @@ -91,7 +88,7 @@ examples:
>
> pinctrl: pinctrl {
> compatible = "aspeed,g5-pinctrl";
> - aspeed,external-nodes = <&gfx &lhc>;
> + aspeed,external-nodes = <&gfx>, <&lhc>;
>
> pinctrl_i2c3_default: i2c3_default {
> function = "I2C3";
> --
> 2.20.1
>
>
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Andrew Jeffery @ 2019-07-16 0:36 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAL_JsqLkOtsAxj9NvNB=EEkH00k-dtNedNY042uuntSmcjhDhA@mail.gmail.com>
On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > data bus if only a single slot is enabled.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > In v2:
> >
> > * Rename to aspeed,sdhci.yaml
> > * Rename sd-controller compatible
> > * Add `maxItems: 1` for reg properties
> > * Move sdhci subnode description to patternProperties
> > * Drop sdhci compatible requirement
> > * #address-cells and #size-cells are required
> > * Prevent additional properties
> > * Implement explicit ranges in example
> > * Remove slot property
> >
> > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > 1 file changed, 90 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > new file mode 100644
> > index 000000000000..67a691c3348c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ASPEED SD/SDIO/eMMC Controller
> > +
> > +maintainers:
> > + - Andrew Jeffery <andrew@aj.id.au>
> > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > +
> > +description: |+
> > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > + only a single slot is enabled.
> > +
> > + The two slots are supported by a common configuration area. As the SDHCIs for
> > + the slots are dependent on the common configuration area, they are described
> > + as child nodes.
> > +
> > +properties:
> > + compatible:
> > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
>
> This is actually a list of 4 strings. Please reformat to 1 per line.
On reflection that's obvious, but also a somewhat subtle interaction with the
preference for no quotes (the obvious caveat being "except where required").
Thanks for pointing it out.
I have been running `make dt_binding_check` and `make dtbs_check` over
these, looks like I need to up my game a bit though. Do you do additional things
in your workflow?
Andrew
^ permalink raw reply
* [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Joel Stanley @ 2019-07-15 23:16 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190715223725.12924-1-robh@kernel.org>
On Mon, 15 Jul 2019 at 22:37, Rob Herring <robh@kernel.org> wrote:
>
> The Aspeed pinctl schema have errors in the 'compatible' schema:
>
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
> properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
> Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
> properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
>
> Flow style sequences have to be quoted if the vales contain ','. Fix
> this by using the more common one line per entry formatting.
>
> properties:
> compatible:
> - enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
> + enum:
> + - aspeed,ast2400-pinctrl
> + - aspeed,g4-pinctrl
Thanks for the fix. However, we've standardised on the first form for
all of our device trees, so we can drop the second compatible string
from the bindings.
I think Andrew already has a patch cooking to do this.
Cheers,
Joel
^ permalink raw reply
* [PATCH] dt-bindings: pinctrl: aspeed: Fix AST2500 example errors
From: Rob Herring @ 2019-07-15 22:48 UTC (permalink / raw)
To: linux-aspeed
The schema examples are now validated against the schema itself. The
AST2500 pinctrl schema has a couple of errors:
Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.example.dt.yaml: \
example-0: $nodename:0: 'example-0' does not match '^(bus|soc|axi|ahb|apb)(@[0-9a-f]+)?$'
Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.example.dt.yaml: \
pinctrl: aspeed,external-nodes: [[1, 2]] is too short
Fixes: 0a617de16730 ("dt-bindings: pinctrl: aspeed: Convert AST2500 bindings to json-schema")
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Joel Stanley <joel@jms.id.au>
Cc: linux-aspeed at lists.ozlabs.org
Cc: linux-gpio at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
index a464cfa0cba3..3e6d85318577 100644
--- a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
@@ -76,9 +76,6 @@ required:
examples:
- |
- compatible = "simple-bus";
- ranges;
-
apb {
compatible = "simple-bus";
#address-cells = <1>;
@@ -91,7 +88,7 @@ examples:
pinctrl: pinctrl {
compatible = "aspeed,g5-pinctrl";
- aspeed,external-nodes = <&gfx &lhc>;
+ aspeed,external-nodes = <&gfx>, <&lhc>;
pinctrl_i2c3_default: i2c3_default {
function = "I2C3";
--
2.20.1
^ permalink raw reply related
* [PATCH] dt-bindings: pinctrl: aspeed: Fix 'compatible' schema errors
From: Rob Herring @ 2019-07-15 22:37 UTC (permalink / raw)
To: linux-aspeed
The Aspeed pinctl schema have errors in the 'compatible' schema:
Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml: \
properties:compatible:enum: ['aspeed', 'ast2400-pinctrl', 'aspeed', 'g4-pinctrl'] has non-unique elements
Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml: \
properties:compatible:enum: ['aspeed', 'ast2500-pinctrl', 'aspeed', 'g5-pinctrl'] has non-unique elements
Flow style sequences have to be quoted if the vales contain ','. Fix
this by using the more common one line per entry formatting.
Fixes: 0a617de16730 ("dt-bindings: pinctrl: aspeed: Convert AST2500 bindings to json-schema")
Fixes: 07457937bb5c ("dt-bindings: pinctrl: aspeed: Convert AST2400 bindings to json-schema")
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Joel Stanley <joel@jms.id.au>
Cc: linux-aspeed at lists.ozlabs.org
Cc: linux-gpio at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml | 4 +++-
.../devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
index 61a110a7db8a..125599a2dc5e 100644
--- a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
@@ -22,7 +22,9 @@ description: |+
properties:
compatible:
- enum: [ aspeed,ast2400-pinctrl, aspeed,g4-pinctrl ]
+ enum:
+ - aspeed,ast2400-pinctrl
+ - aspeed,g4-pinctrl
patternProperties:
'^.*$':
diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
index cf561bd55128..a464cfa0cba3 100644
--- a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.yaml
@@ -22,7 +22,9 @@ description: |+
properties:
compatible:
- enum: [ aspeed,ast2500-pinctrl, aspeed,g5-pinctrl ]
+ enum:
+ - aspeed,ast2500-pinctrl
+ - aspeed,g5-pinctrl
aspeed,external-nodes:
minItems: 2
maxItems: 2
--
2.20.1
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Rob Herring @ 2019-07-15 22:16 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190712033214.24713-2-andrew@aj.id.au>
On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
> The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> data bus if only a single slot is enabled.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> In v2:
>
> * Rename to aspeed,sdhci.yaml
> * Rename sd-controller compatible
> * Add `maxItems: 1` for reg properties
> * Move sdhci subnode description to patternProperties
> * Drop sdhci compatible requirement
> * #address-cells and #size-cells are required
> * Prevent additional properties
> * Implement explicit ranges in example
> * Remove slot property
>
> .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
>
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> new file mode 100644
> index 000000000000..67a691c3348c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED SD/SDIO/eMMC Controller
> +
> +maintainers:
> + - Andrew Jeffery <andrew@aj.id.au>
> + - Ryan Chen <ryanchen.aspeed@gmail.com>
> +
> +description: |+
> + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> + only a single slot is enabled.
> +
> + The two slots are supported by a common configuration area. As the SDHCIs for
> + the slots are dependent on the common configuration area, they are described
> + as child nodes.
> +
> +properties:
> + compatible:
> + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
This is actually a list of 4 strings. Please reformat to 1 per line.
Rob
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Rob Herring @ 2019-07-15 13:26 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <5c831fd3-d0e2-474b-8a6e-8f51f92fbdf8@www.fastmail.com>
On Sun, Jul 14, 2019 at 8:30 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Fri, 12 Jul 2019, at 22:41, Maxime Ripard wrote:
> > Hi,
> >
> > On Fri, Jul 12, 2019 at 01:02:13PM +0930, Andrew Jeffery wrote:
> > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > data bus if only a single slot is enabled.
> > >
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > > In v2:
> > >
> > > * Rename to aspeed,sdhci.yaml
> > > * Rename sd-controller compatible
> > > * Add `maxItems: 1` for reg properties
> > > * Move sdhci subnode description to patternProperties
> > > * Drop sdhci compatible requirement
> > > * #address-cells and #size-cells are required
> > > * Prevent additional properties
> > > * Implement explicit ranges in example
> > > * Remove slot property
> > >
> > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > 1 file changed, 90 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > new file mode 100644
> > > index 000000000000..67a691c3348c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > @@ -0,0 +1,90 @@
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ASPEED SD/SDIO/eMMC Controller
> > > +
> > > +maintainers:
> > > + - Andrew Jeffery <andrew@aj.id.au>
> > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > +
> > > +description: |+
> > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > + only a single slot is enabled.
> > > +
> > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > + the slots are dependent on the common configuration area, they are described
> > > + as child nodes.
> > > +
> > > +properties:
> > > + compatible:
> > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> > > + reg:
> > > + maxItems: 1
> > > + description: Common configuration registers
> > > + ranges: true
> > > + clocks:
> > > + maxItems: 1
> > > + description: The SD/SDIO controller clock gate
> >
> > #address-cells and #size-cells have not been described here.
> >
> > > +patternProperties:
> > > + "^sdhci@[0-9a-f]+$":
> > > + type: object
> > > + properties:
> > > + compatible:
> > > + enum: [ aspeed,ast2400-sdhci, aspeed,ast2500-sdhci ]
> > > + reg:
> > > + maxItems: 1
> > > + description: The SDHCI registers
> > > + clocks:
> > > + maxItems: 1
> > > + description: The SD bus clock
> > > + interrupts:
> > > + maxItems: 1
> > > + description: The SD interrupt shared between both slots
> > > + required:
> > > + - compatible
> > > + - reg
> > > + - clocks
> > > + - interrupts
> > > +
> > > +additionalProperties: false
> >
> > But that means that it will generate a warning in your DT if you ever
> > use them.
> >
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - "#address-cells"
> > > + - "#size-cells"
> > > + - ranges
> > > + - clocks
> > > +
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/clock/aspeed-clock.h>
> > > + sdc at 1e740000 {
> > > + compatible = "aspeed,ast2500-sd-controller";
> > > + reg = <0x1e740000 0x100>;
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> >
> > Starting with your example.
>
> Heh, right. Thanks. I was inspecting the output of the `dt_binding_check` and
> `dtbs_check` make targets, though maybe I overlooked this. The aspeed dtsis
> do generate a quite a number of warnings which make it hard to parse, so I'm
> going to send a series to clean that up too.
FYI, This will run checks with only the schema file you specify:
make dtbs_check DT_SCHEMA_FILES=path/to/schema/file
Rob
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox