* [PATCH v2] gpiolib: Preserve desc->flags when setting state
From: Chris Packham @ 2019-07-04 4:20 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, ricardo.ribalda
Cc: linux-gpio, linux-kernel, Chris Packham
desc->flags may already have values set by of_gpiochip_add() so make
sure that this isn't undone when setting the initial direction.
Fixes: 3edfb7bd76bd1cba ("gpiolib: Show correct direction from the beginning")
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Notes:
Changes in v2:
- add braces to avoid ambiguious else warning
drivers/gpio/gpiolib.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e013d417a936..5ac57264171b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1392,12 +1392,13 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
for (i = 0; i < chip->ngpio; i++) {
struct gpio_desc *desc = &gdev->descs[i];
- if (chip->get_direction && gpiochip_line_is_valid(chip, i))
- desc->flags = !chip->get_direction(chip, i) ?
- (1 << FLAG_IS_OUT) : 0;
- else
- desc->flags = !chip->direction_input ?
- (1 << FLAG_IS_OUT) : 0;
+ if (chip->get_direction && gpiochip_line_is_valid(chip, i)) {
+ if (!chip->get_direction(chip, i))
+ set_bit(FLAG_IS_OUT, &desc->flags);
+ } else {
+ if (!chip->direction_input)
+ set_bit(FLAG_IS_OUT, &desc->flags);
+ }
}
acpi_gpiochip_add(chip);
--
2.22.0
^ permalink raw reply related
* Re: [GIT PULL] GPIO fix for v5.2
From: pr-tracker-bot @ 2019-07-04 2:15 UTC (permalink / raw)
To: Linus Walleij; +Cc: Linus Torvalds, open list:GPIO SUBSYSTEM, linux-kernel
In-Reply-To: <CACRpkdZBhUF7C_+vPc6tkasBk5DAGh01g3eu8OYQ16QBehUZWw@mail.gmail.com>
The pull request you sent on Wed, 3 Jul 2019 14:30:51 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git tags/gpio-v5.2-4
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/179c96d9f715a619811ded5dcbd35b1fee8caf69
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v2 0/8] pinctrl: aspeed: Preparation for AST2600
From: Andrew Jeffery @ 2019-07-04 0:28 UTC (permalink / raw)
To: Linus Walleij
Cc: open list:GPIO SUBSYSTEM, Ryan Chen, Rob Herring, Mark Rutland,
Joel Stanley, linux-aspeed, OpenBMC Maillist,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdaxiFR3ezt4FzhRxpqc4DYYjsbBeysPUaaQH+_QgYjudw@mail.gmail.com>
On Wed, 3 Jul 2019, at 18:40, Linus Walleij wrote:
> Hi Andrew,
>
> On Fri, Jun 28, 2019 at 4:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Hello!
> >
> > The ASPEED AST2600 is in the pipeline, and we have enough information to start
> > preparing to upstream support for it. This series lays some ground work;
> > splitting the bindings and dicing the implementation up a little further to
> > facilitate differences between the 2600 and previous SoC generations.
> >
> > v2 addresses Rob's comments on the bindings conversion patches. v1 can be found
> > here:
>
> I have applied this series, I had to strip some changes of the header
> because it was based on some SPDX cleanups upstream but no
> big deal I think. Check the result please.
Thanks. Have you pushed the branch yet? I just fetched your pinctrl tree
and can't see the patches.
Andrew
^ permalink raw reply
* Re: [PATCH 2/3 linux,dev-5.1 v1] ARM: dts: aspeed: Add SGPIO driver
From: Andrew Jeffery @ 2019-07-04 0:06 UTC (permalink / raw)
To: Hongwei Zhang, Bartosz Golaszewski, Joel Stanley, Linus Walleij
Cc: linux-gpio, linux-arm-kernel, linux-aspeed, linux-kernel
In-Reply-To: <1562188172-23178-1-git-send-email-hongweiz@ami.com>
Hello Hongwei,
As this is patch is sent to the upstream lists (linux-gpio@ etc) please
drop the OpenBMC-specific "linux,dev-5.1" from the subject.
Also, it looks like you may have manually added the series revision (v1).
For the record you can make `git format-patch` do this for you with the
`-v`option (e.g. if you really want it here, `-v 1`).
On Thu, 4 Jul 2019, at 07:09, Hongwei Zhang wrote:
> Add SGPIO driver support for Aspeed AST2500 SoC.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> drivers/gpio/sgpio-aspeed.c | 470 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 470 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..108ed13
> --- /dev/null
> +++ b/drivers/gpio/sgpio-aspeed.c
> @@ -0,0 +1,470 @@
> +/*
> + * Copyright 2019 American Megatrends International LLC.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
You should use the SPDX license identifier here rather than the GPL
blurb, and it should be the first line of the file. Keep your copyright
line in place though:
// SPDX-License-Identifier: GPL-2.0-or-later
// Copyright 2019 American Megatrends International LLC.
> + */
> +
> +#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;
> + }
> + BUG_ON(1);
This isn't appropriate - we shouldn't take down the kernel on a
faulty peripheral access. Please change this to WARN().
> +}
> +
> +#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 inline bool have_gpio(struct aspeed_sgpio *gpio, unsigned int
> offset)
> +{
> + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> + unsigned int group = GPIO_OFFSET(offset) / 8;
> +
> + return bank->names[group][0] != '\0';
Lets just drop have_gpio() altogether, it's a contiguous set of 80 GPIOs.
At best this should just be:
static inline bool have_gpio(struct aspeed_sgpio *gpio, unsigned int offset)
{
return offset < NR_SGPIO;
}
But lets just assume that we've properly configured the gpio subsystem
for the controller and remove it completely.
> +}
> +
> +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)
No need to split this out from aspeed_sgpio_set() below. Separating
the implementation was necessary in the parallel GPIO driver for reasons
that aren't relevant here.
> +{
> + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> + void __iomem *addr;
> + u32 reg;
> +
> + addr = bank_reg(gpio, bank, reg_val);
> +
> + if (val)
> + reg |= GPIO_BIT(offset);
> + else
> + reg &= ~GPIO_BIT(offset);
> +
> + iowrite32(reg, addr);
> +}
> +
> +static void aspeed_sgpio_set(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);
> +
> + __aspeed_sgpio_set(gc, offset, val);
> +
> + 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 */
Right, but with your implementation below you can never mark them as
output.
> + return 0;
> +}
> +
> +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> int offset)
> +{
> + /* By default all SGPIO Pins are input */
> + return 1;
As above. Given my understanding of SGPIO, I think you should be
implementing both dir_in() and dir_out(), and capturing which state
userspace "wants" the GPIO to be in, and directing reads/writes to the
DATA_READ/DATA_VALUE registers as appropriate. There's no state
we need to modify in the hardware, but that doesn't mean we shouldn't
capture the intent of userspace at all.
> +
> +}
> +
> +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)
> +{
> + if (!have_gpio(gpiochip_get_data(chip), offset))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_sgpio_of_table[] = {
> + { .compatible = "aspeed,ast2400-sgpio", .data = NULL, },
> + { .compatible = "aspeed,ast2500-sgpio", .data = NULL,},
You can drop the assignment to data.
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> +
> +static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> +{
> + const struct of_device_id *gpio_id;
> + struct aspeed_sgpio *gpio;
> + struct resource *res;
> + int rc, i;
> +
> + 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_id = of_match_node(aspeed_sgpio_of_table, pdev->dev.of_node);
> + if (!gpio_id)
> + return -EINVAL;
gpio_id isn't used, so you can drop the of_match_node() above.
> +
> + gpio->chip.parent = &pdev->dev;
> + gpio->chip.ngpio = NR_SGPIO;
> + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> + gpio->chip.direction_output = NULL;
We can do outputs too - we shouldn't be omitting the direction_output
callback, see the discussion above about dir_in()/dir_out()/get_direction()
Andrew
> + 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] gpiolib: Preserve desc->flags when setting state
From: Chris Packham @ 2019-07-03 22:57 UTC (permalink / raw)
To: linus.walleij, bgolaszewski, ricardo.ribalda
Cc: linux-gpio, linux-kernel, Chris Packham
desc->flags may already have values set by of_gpiochip_add() so make
sure that this isn't undone when setting the initial direction.
Fixes: 3edfb7bd76bd1cba ("gpiolib: Show correct direction from the beginning")
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
drivers/gpio/gpiolib.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e013d417a936..cc651e3dea54 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1393,11 +1393,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
struct gpio_desc *desc = &gdev->descs[i];
if (chip->get_direction && gpiochip_line_is_valid(chip, i))
- desc->flags = !chip->get_direction(chip, i) ?
- (1 << FLAG_IS_OUT) : 0;
+ if (!chip->get_direction(chip, i))
+ set_bit(FLAG_IS_OUT, &desc->flags);
else
- desc->flags = !chip->direction_input ?
- (1 << FLAG_IS_OUT) : 0;
+ if (!chip->direction_input)
+ set_bit(FLAG_IS_OUT, &desc->flags);
}
acpi_gpiochip_add(chip);
--
2.22.0
^ permalink raw reply related
* Re: gpio desc flags being lost
From: Chris Packham @ 2019-07-03 21:30 UTC (permalink / raw)
To: Linus Walleij, Ricardo Ribalda Delgado
Cc: Bartosz Golaszewski, linux-gpio@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <f9eb3387ed384676b0b298e4da7eeaf0@svr-chch-ex1.atlnz.lc>
On 3/07/19 9:29 PM, Chris Packham wrote:
> On 3/07/19 6:28 PM, Linus Walleij wrote:
>> On Wed, Jul 3, 2019 at 7:35 AM Chris Packham
>> <Chris.Packham@alliedtelesis.co.nz> wrote:
>>
>>> Doing a bit of debugging so far I see that after startup the desc->flags
>>> for those gpios is 0. But for the hogged ones it should be 0x800 (or 0x801).
>>
>> Yeah that is wrong.
>>
>>> I'll do some proper bisecting tomorrow, but figured you might want to
>>> know sooner rather than later.
>>
>> Thanks, I have another critical GPIO fix queued so would be great if we
>> can fix this too before v5.2 is released.
>
> Bit of an update. v4.19.54 works, v5.1.15 is broken. I'll keep bisecting
> between those tomorrow.
>
The problem is caused by commit 3edfb7bd76bd1cba ("gpiolib: Show correct
direction from the beginning"). I'll see if I can whip up a patch to fix it.
^ permalink raw reply
* [PATCH 2/3 linux,dev-5.1 v1] ARM: dts: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-03 21:09 UTC (permalink / raw)
To: Bartosz Golaszewski, Joel Stanley, Andrew Jeffery, Linus Walleij
Cc: Hongwei Zhang, linux-gpio, linux-arm-kernel, linux-aspeed,
linux-kernel
Add SGPIO driver support for Aspeed AST2500 SoC.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
drivers/gpio/sgpio-aspeed.c | 470 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 470 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..108ed13
--- /dev/null
+++ b/drivers/gpio/sgpio-aspeed.c
@@ -0,0 +1,470 @@
+/*
+ * Copyright 2019 American Megatrends International LLC.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#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;
+ }
+ 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 inline bool have_gpio(struct aspeed_sgpio *gpio, unsigned int offset)
+{
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+ unsigned int group = GPIO_OFFSET(offset) / 8;
+
+ return bank->names[group][0] != '\0';
+}
+
+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);
+ void __iomem *addr;
+ u32 reg;
+
+ addr = bank_reg(gpio, bank, reg_val);
+
+ if (val)
+ reg |= GPIO_BIT(offset);
+ else
+ reg &= ~GPIO_BIT(offset);
+
+ iowrite32(reg, addr);
+}
+
+static void aspeed_sgpio_set(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);
+
+ __aspeed_sgpio_set(gc, offset, val);
+
+ 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_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ /* By default all SGPIO Pins are input */
+ return 1;
+
+}
+
+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)
+{
+ if (!have_gpio(gpiochip_get_data(chip), offset))
+ return -ENODEV;
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_sgpio_of_table[] = {
+ { .compatible = "aspeed,ast2400-sgpio", .data = NULL, },
+ { .compatible = "aspeed,ast2500-sgpio", .data = NULL,},
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
+
+static int __init aspeed_sgpio_probe(struct platform_device *pdev)
+{
+ const struct of_device_id *gpio_id;
+ struct aspeed_sgpio *gpio;
+ struct resource *res;
+ int rc, i;
+
+ 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_id = of_match_node(aspeed_sgpio_of_table, pdev->dev.of_node);
+ if (!gpio_id)
+ return -EINVAL;
+
+ gpio->chip.parent = &pdev->dev;
+ gpio->chip.ngpio = NR_SGPIO;
+ gpio->chip.direction_input = aspeed_sgpio_dir_in;
+ gpio->chip.direction_output = NULL;
+ 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
* Re: [PATCH v3] driver: core: Allow subsystems to continue deferring probe
From: Greg Kroah-Hartman @ 2019-07-03 19:28 UTC (permalink / raw)
To: Thierry Reding
Cc: Rafael J . Wysocki, Linus Walleij, Rob Herring, linux-gpio,
linux-kernel
In-Reply-To: <20190621151725.20414-1-thierry.reding@gmail.com>
On Fri, Jun 21, 2019 at 05:17:25PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Some subsystems, such as pinctrl, allow continuing to defer probe
> indefinitely. This is useful for devices that depend on resources
> provided by devices that are only probed after the init stage.
>
> One example of this can be seen on Tegra, where the DPAUX hardware
> contains pinmuxing controls for pins that it shares with an I2C
> controller. The I2C controller is typically used for communication
> with a monitor over HDMI (DDC). However, other instances of the I2C
> controller are used to access system critical components, such as a
> PMIC. The I2C controller driver will therefore usually be a builtin
> driver, whereas the DPAUX driver is part of the display driver that
> is loaded from a module to avoid bloating the kernel image with all
> of the DRM/KMS subsystem.
>
> In this particular case the pins used by this I2C/DDC controller
> become accessible very late in the boot process. However, since the
> controller is only used in conjunction with display, that's not an
> issue.
>
> Unfortunately the driver core currently outputs a warning message
> when a device fails to get the pinctrl before the end of the init
> stage. That can be confusing for the user because it may sound like
> an unwanted error occurred, whereas it's really an expected and
> harmless situation.
>
> In order to eliminate this warning, this patch allows callers of the
> driver_deferred_probe_check_state() helper to specify that they want
> to continue deferring probe, regardless of whether we're past the
> init stage or not. All of the callers of that function are updated
> for the new signature, but only the pinctrl subsystem passes a true
> value in the new persist parameter if appropriate.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes in v3:
> - add new function rather than extend the existing function with flags
Much nicer, thanks for making the changes!
greg k-h
^ permalink raw reply
* Re: [PATCH v1] pinctrl: baytrail: Use GENMASK() consistently
From: Mika Westerberg @ 2019-07-03 16:38 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-gpio, Linus Walleij
In-Reply-To: <20190703151554.30454-1-andriy.shevchenko@linux.intel.com>
On Wed, Jul 03, 2019 at 06:15:54PM +0300, Andy Shevchenko wrote:
> Use GENMASK() macro for all definitions where it's appropriate.
> No functional change intended.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v1 1/4] pinctrl: baytrail: Use devm_platform_ioremap_resource()
From: Mika Westerberg @ 2019-07-03 16:36 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-gpio, Linus Walleij
In-Reply-To: <20190703145615.74768-1-andriy.shevchenko@linux.intel.com>
On Wed, Jul 03, 2019 at 05:56:12PM +0300, Andy Shevchenko wrote:
> Use the new helper that wraps the calls to platform_get_resource()
> and devm_ioremap_resource() together.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
For the whole series,
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply
* [PATCH v1] pinctrl: baytrail: Use GENMASK() consistently
From: Andy Shevchenko @ 2019-07-03 15:15 UTC (permalink / raw)
To: Mika Westerberg, linux-gpio, Linus Walleij; +Cc: Andy Shevchenko
Use GENMASK() macro for all definitions where it's appropriate.
No functional change intended.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-baytrail.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 4fc4f9658484..14a81943c5ab 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -37,6 +37,7 @@
/* BYT_CONF0_REG register bits */
#define BYT_IODEN BIT(31)
#define BYT_DIRECT_IRQ_EN BIT(27)
+#define BYT_TRIG_MASK GENMASK(26, 24)
#define BYT_TRIG_NEG BIT(26)
#define BYT_TRIG_POS BIT(25)
#define BYT_TRIG_LVL BIT(24)
@@ -45,31 +46,28 @@
#define BYT_GLITCH_F_SLOW_CLK BIT(17)
#define BYT_GLITCH_F_FAST_CLK BIT(16)
#define BYT_PULL_STR_SHIFT 9
-#define BYT_PULL_STR_MASK (3 << BYT_PULL_STR_SHIFT)
+#define BYT_PULL_STR_MASK GENMASK(10, 9)
#define BYT_PULL_STR_2K (0 << BYT_PULL_STR_SHIFT)
#define BYT_PULL_STR_10K (1 << BYT_PULL_STR_SHIFT)
#define BYT_PULL_STR_20K (2 << BYT_PULL_STR_SHIFT)
#define BYT_PULL_STR_40K (3 << BYT_PULL_STR_SHIFT)
#define BYT_PULL_ASSIGN_SHIFT 7
-#define BYT_PULL_ASSIGN_MASK (3 << BYT_PULL_ASSIGN_SHIFT)
+#define BYT_PULL_ASSIGN_MASK GENMASK(8, 7)
#define BYT_PULL_ASSIGN_UP (1 << BYT_PULL_ASSIGN_SHIFT)
#define BYT_PULL_ASSIGN_DOWN (2 << BYT_PULL_ASSIGN_SHIFT)
-#define BYT_PIN_MUX 0x07
+#define BYT_PIN_MUX GENMASK(2, 0)
/* BYT_VAL_REG register bits */
+#define BYT_DIR_MASK GENMASK(2, 1)
#define BYT_INPUT_EN BIT(2) /* 0: input enabled (active low)*/
#define BYT_OUTPUT_EN BIT(1) /* 0: output enabled (active low)*/
#define BYT_LEVEL BIT(0)
-#define BYT_DIR_MASK (BIT(1) | BIT(2))
-#define BYT_TRIG_MASK (BIT(26) | BIT(25) | BIT(24))
-
-#define BYT_CONF0_RESTORE_MASK (BYT_DIRECT_IRQ_EN | BYT_TRIG_MASK | \
- BYT_PIN_MUX)
+#define BYT_CONF0_RESTORE_MASK (BYT_DIRECT_IRQ_EN | BYT_TRIG_MASK | BYT_PIN_MUX)
#define BYT_VAL_RESTORE_MASK (BYT_DIR_MASK | BYT_LEVEL)
/* BYT_DEBOUNCE_REG bits */
-#define BYT_DEBOUNCE_PULSE_MASK 0x7
+#define BYT_DEBOUNCE_PULSE_MASK GENMASK(2, 0)
#define BYT_DEBOUNCE_PULSE_375US 1
#define BYT_DEBOUNCE_PULSE_750US 2
#define BYT_DEBOUNCE_PULSE_1500US 3
--
2.20.1
^ permalink raw reply related
* [PATCH v1 3/4] pinctrl: intel: Use devm_platform_ioremap_resource()
From: Andy Shevchenko @ 2019-07-03 14:56 UTC (permalink / raw)
To: Mika Westerberg, linux-gpio, Linus Walleij; +Cc: Andy Shevchenko
In-Reply-To: <20190703145615.74768-1-andriy.shevchenko@linux.intel.com>
Use the new helper that wraps the calls to platform_get_resource()
and devm_ioremap_resource() together.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-intel.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 13e4d61f14a4..dc6ef88790ae 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1313,15 +1313,12 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
for (i = 0; i < pctrl->ncommunities; i++) {
struct intel_community *community = &pctrl->communities[i];
- struct resource *res;
void __iomem *regs;
u32 padbar;
*community = pctrl->soc->communities[i];
- res = platform_get_resource(pdev, IORESOURCE_MEM,
- community->barno);
- regs = devm_ioremap_resource(&pdev->dev, res);
+ regs = devm_platform_ioremap_resource(pdev, community->barno);
if (IS_ERR(regs))
return PTR_ERR(regs);
--
2.20.1
^ permalink raw reply related
* [PATCH v1 4/4] pinctrl: merrifield: Use devm_platform_ioremap_resource()
From: Andy Shevchenko @ 2019-07-03 14:56 UTC (permalink / raw)
To: Mika Westerberg, linux-gpio, Linus Walleij; +Cc: Andy Shevchenko
In-Reply-To: <20190703145615.74768-1-andriy.shevchenko@linux.intel.com>
Use the new helper that wraps the calls to platform_get_resource()
and devm_ioremap_resource() together.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-merrifield.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-merrifield.c b/drivers/pinctrl/intel/pinctrl-merrifield.c
index 4b65e1296b8a..04ca8ae95df8 100644
--- a/drivers/pinctrl/intel/pinctrl-merrifield.c
+++ b/drivers/pinctrl/intel/pinctrl-merrifield.c
@@ -885,7 +885,6 @@ static int mrfld_pinctrl_probe(struct platform_device *pdev)
{
struct mrfld_family *families;
struct mrfld_pinctrl *mp;
- struct resource *mem;
void __iomem *regs;
size_t nfamilies;
unsigned int i;
@@ -897,8 +896,7 @@ static int mrfld_pinctrl_probe(struct platform_device *pdev)
mp->dev = &pdev->dev;
raw_spin_lock_init(&mp->lock);
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- regs = devm_ioremap_resource(&pdev->dev, mem);
+ regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(regs))
return PTR_ERR(regs);
--
2.20.1
^ permalink raw reply related
* [PATCH v1 2/4] pinctrl: cherryview: Use devm_platform_ioremap_resource()
From: Andy Shevchenko @ 2019-07-03 14:56 UTC (permalink / raw)
To: Mika Westerberg, linux-gpio, Linus Walleij; +Cc: Andy Shevchenko
In-Reply-To: <20190703145615.74768-1-andriy.shevchenko@linux.intel.com>
Use the new helper that wraps the calls to platform_get_resource()
and devm_ioremap_resource() together.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-cherryview.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index d0740714b3cb..6e7da421b6c9 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1689,7 +1689,6 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
{
struct chv_pinctrl *pctrl;
struct acpi_device *adev;
- struct resource *res;
acpi_status status;
int ret, irq, i;
@@ -1717,8 +1716,7 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
if (!pctrl->saved_pin_context)
return -ENOMEM;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- pctrl->regs = devm_ioremap_resource(&pdev->dev, res);
+ pctrl->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(pctrl->regs))
return PTR_ERR(pctrl->regs);
--
2.20.1
^ permalink raw reply related
* [PATCH v1 1/4] pinctrl: baytrail: Use devm_platform_ioremap_resource()
From: Andy Shevchenko @ 2019-07-03 14:56 UTC (permalink / raw)
To: Mika Westerberg, linux-gpio, Linus Walleij; +Cc: Andy Shevchenko
Use the new helper that wraps the calls to platform_get_resource()
and devm_ioremap_resource() together.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-baytrail.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index bfde1c710bd9..4fc4f9658484 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1582,12 +1582,10 @@ static int byt_set_soc_data(struct byt_gpio *vg,
for (i = 0; i < soc_data->ncommunities; i++) {
struct byt_community *comm = vg->communities_copy + i;
- struct resource *mem_rc;
*comm = vg->soc_data->communities[i];
- mem_rc = platform_get_resource(vg->pdev, IORESOURCE_MEM, 0);
- comm->reg_base = devm_ioremap_resource(&vg->pdev->dev, mem_rc);
+ comm->reg_base = devm_platform_ioremap_resource(vg->pdev, 0);
if (IS_ERR(comm->reg_base))
return PTR_ERR(comm->reg_base);
}
--
2.20.1
^ permalink raw reply related
* Re: [RFC/RFT v3 04/14] clk: meson: eeclk: add setup callback
From: Jerome Brunet @ 2019-07-03 14:17 UTC (permalink / raw)
To: Neil Armstrong, khilman
Cc: linux-arm-kernel, linux-amlogic, linux-kernel, linux-clk,
martin.blumenstingl, linux-gpio, Neil Armstrong
In-Reply-To: <20190701091258.3870-5-narmstrong@baylibre.com>
On Mon 01 Jul 2019 at 11:12, Neil Armstrong <narmstrong@baylibre.com> wrote:
> Add a setup() callback in the eeclk structure, to call an optional
> call() function at end of eeclk probe to setup clocks.
>
> It's used for the G12A clock controller to setup the CPU clock
> notifiers.
I'd prefer if you implement the probe function in the related controller
have this probe function call meson_eeclkc_probe() for the common part
In your case, I suppose it means implementing the g12a controller probe
to deal with the notifiers
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> drivers/clk/meson/meson-eeclk.c | 6 ++++++
> drivers/clk/meson/meson-eeclk.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/clk/meson/meson-eeclk.c b/drivers/clk/meson/meson-eeclk.c
> index 6ba2094be257..81fd2abcd173 100644
> --- a/drivers/clk/meson/meson-eeclk.c
> +++ b/drivers/clk/meson/meson-eeclk.c
> @@ -61,6 +61,12 @@ int meson_eeclkc_probe(struct platform_device *pdev)
> }
> }
>
> + if (data->setup) {
> + ret = data->setup(pdev);
> + if (ret)
> + return ret;
> + }
> +
> return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> data->hw_onecell_data);
> }
> diff --git a/drivers/clk/meson/meson-eeclk.h b/drivers/clk/meson/meson-eeclk.h
> index 9ab5d6fa7ccb..7fdf424f71a6 100644
> --- a/drivers/clk/meson/meson-eeclk.h
> +++ b/drivers/clk/meson/meson-eeclk.h
> @@ -20,6 +20,7 @@ struct meson_eeclkc_data {
> const struct reg_sequence *init_regs;
> unsigned int init_count;
> struct clk_hw_onecell_data *hw_onecell_data;
> + int (*setup)(struct platform_device *pdev);
> };
>
> int meson_eeclkc_probe(struct platform_device *pdev);
> --
> 2.21.0
^ permalink raw reply
* Re: [RFC/RFT v3 04/14] clk: meson: eeclk: add setup callback
From: Martin Blumenstingl @ 2019-07-03 12:57 UTC (permalink / raw)
To: Jerome Brunet
Cc: Neil Armstrong, khilman, linux-arm-kernel, linux-amlogic,
linux-kernel, linux-clk, linux-gpio, amergnat
In-Reply-To: <1jo92b70ko.fsf@starbuckisacylon.baylibre.com>
On Wed, Jul 3, 2019 at 2:40 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Wed 03 Jul 2019 at 13:45, Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> > On 03/07/2019 01:16, Martin Blumenstingl wrote:
> >> +Cc Alexandre Mergnat
> >>
> >> On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >>>
> >>> Add a setup() callback in the eeclk structure, to call an optional
> >>> call() function at end of eeclk probe to setup clocks.
> >>>
> >>> It's used for the G12A clock controller to setup the CPU clock notifiers.
> >>>
> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >> this will probably work fine, but I want do double check first
> >>
> >> are we planning to get rid of meson-eeclk (mid-term)?
> >
> > AFAIK no, but maybe I'm not aware of it !
> >
> > Neil
> >
> >> Alex has some patches to get rid of all these IN_PREFIX logic.
>
> The prefix logic will go away with Alex's rework, so are the input clock
> But meson-eeclk, which is just a common probe function do avoid
> repeating the same things over and over, will stay
OK, thank you for clarifying this
> >> I'm asking because if we want to get rid of meson-eeclk it may be the
>
> May I ask why ?
I only remember that Stephen asked us to get rid of something in our clock code
I was under the impression that it was meson-eeclk, but I cannot find
it anymore (that means I'm mixing it up with some other topic)
^ permalink raw reply
* Re: [RFC/RFT v3 09/14] arm64: dts: move common G12A & G12B modes to meson-g12-common.dtsi
From: Jerome Brunet @ 2019-07-03 12:48 UTC (permalink / raw)
To: Neil Armstrong, Martin Blumenstingl
Cc: khilman, linux-arm-kernel, linux-amlogic, linux-kernel, linux-clk,
linux-gpio
In-Reply-To: <e49bfb8f-23e9-c834-62c7-a40528a2ef06@baylibre.com>
On Wed 03 Jul 2019 at 13:51, Neil Armstrong <narmstrong@baylibre.com> wrote:
> On 03/07/2019 01:54, Martin Blumenstingl wrote:
>> Hi Neil,
>>
[...]
>> does it make sense to name this file "meson-g12a-g12b-sm1-common.dtsi" instead?
>> do you know whether there will be a successor to G12B and what it's
>> code-name will be?
>
> meson-g12a-g12b-sm1-common seems a bit long to me...
+1 ... and what if the generation after that is compatible as well ? We
extend the name again ? Such naming scheme does not scale.
meson-g12-common.dtsi looks good to me. IMO, The fact the sm1 dtsi includes
the file is enough to understand that sm1 derive from the g12a/b
>
> We don't have naming of the future SoCs, since SM1 is only available on
> prototypes yet.
>
> Neil
>
>>
>>
>> Martin
>>
^ permalink raw reply
* Re: [RFC/RFT v3 04/14] clk: meson: eeclk: add setup callback
From: Jerome Brunet @ 2019-07-03 12:40 UTC (permalink / raw)
To: Neil Armstrong, Martin Blumenstingl
Cc: khilman, linux-arm-kernel, linux-amlogic, linux-kernel, linux-clk,
linux-gpio, amergnat
In-Reply-To: <301695b6-52ba-92b1-ca1a-d4d587b33eeb@baylibre.com>
On Wed 03 Jul 2019 at 13:45, Neil Armstrong <narmstrong@baylibre.com> wrote:
> On 03/07/2019 01:16, Martin Blumenstingl wrote:
>> +Cc Alexandre Mergnat
>>
>> On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>>
>>> Add a setup() callback in the eeclk structure, to call an optional
>>> call() function at end of eeclk probe to setup clocks.
>>>
>>> It's used for the G12A clock controller to setup the CPU clock notifiers.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> this will probably work fine, but I want do double check first
>>
>> are we planning to get rid of meson-eeclk (mid-term)?
>
> AFAIK no, but maybe I'm not aware of it !
>
> Neil
>
>> Alex has some patches to get rid of all these IN_PREFIX logic.
The prefix logic will go away with Alex's rework, so are the input clock
But meson-eeclk, which is just a common probe function do avoid
repeating the same things over and over, will stay
>> I'm asking because if we want to get rid of meson-eeclk it may be the
May I ask why ?
>> time to do so now to have less logic to migrate later on
>>
>>
>> Martin
>>
^ permalink raw reply
* Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains
From: Linus Walleij @ 2019-07-03 12:39 UTC (permalink / raw)
To: Brian Masney
Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Thomas Gleixner,
Marc Zyngier, Lina Iyer, Jon Hunter, Sowjanya Komatineni,
Bitan Biswas, linux-tegra, David Daney, Masahiro Yamada,
Thierry Reding
In-Reply-To: <20190703092236.GA15393@onstation.org>
On Wed, Jul 3, 2019 at 11:22 AM Brian Masney <masneyb@onstation.org> wrote:
> On Mon, Jun 24, 2019 at 03:25:28PM +0200, Linus Walleij wrote:
> > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> > {
> > + struct irq_domain *domain = chip->irq.domain;
> > +
> > if (!gpiochip_irqchip_irq_valid(chip, offset))
> > return -ENXIO;
> >
> > - return irq_create_mapping(chip->irq.domain, offset);
> > + if (irq_domain_is_hierarchy(domain)) {
> > + struct irq_fwspec spec;
> > +
> > + spec.fwnode = domain->fwnode;
> > + spec.param_count = 2;
> > + spec.param[0] = offset;
> > + spec.param[1] = IRQ_TYPE_NONE;
> > +
> > + return irq_create_fwspec_mapping(&spec);
> > + }
>
> spmi-gpio's to_irq() needs to add one to the offset:
>
> static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
> {
> struct pmic_gpio_state *state = gpiochip_get_data(chip);
> struct irq_fwspec fwspec;
>
> fwspec.fwnode = state->fwnode;
> fwspec.param_count = 2;
> fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET;
> /*
> * Set the type to a safe value temporarily. This will be overwritten
> * later with the proper value by irq_set_type.
> */
> fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
>
> return irq_create_fwspec_mapping(&fwspec);
> }
>
> ssbi-gpio will have the same problem as well.
>
> What do you think about adding a new field to the struct gpio_irq_chip
> inside the CONFIG_IRQ_DOMAIN_HIERARCHY ifdef called something like
> to_irq_offset? (I'm bad at naming things.)
I think to cover Lina's need and following the direction set out
by Thierry's desire to have callback so we can control the
parent-to-child translation with code, it might be best to have
an optional callback for translating fwspec.param[0].
Thierry seems to need exactly this for the Tegra driver to,
I think that is why it has custom ops today.
> Also, instead of hardcoding IRQ_TYPE_NONE, what do you think about using
> the default_type field that's available?
I want to get rid of the .default_type in the long run, it is
nominally only for board files where the .set_type() isn't
ever getting called. For anything modern, the .set_type()
callback is always called before any irq is used.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] gpio: pl061: drop duplicate printing of device name
From: Linus Walleij @ 2019-07-03 12:35 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult
Cc: linux-kernel@vger.kernel.org, Bartosz Golaszewski,
open list:GPIO SUBSYSTEM
In-Reply-To: <1562146944-4162-1-git-send-email-info@metux.net>
On Wed, Jul 3, 2019 at 11:42 AM Enrico Weigelt, metux IT consult
<info@metux.net> wrote:
> From: Enrico Weigelt <info@metux.net>
>
> The dev_info() call already prints the device name, so there's
> no need to explicitly include it in the message for second time.
>
> Signed-off-by: Enrico Weigelt <info@metux.net>
True. Patch applied.
Yours,
Linus Walleij
^ permalink raw reply
* [GIT PULL] GPIO fix for v5.2
From: Linus Walleij @ 2019-07-03 12:30 UTC (permalink / raw)
To: Linus Torvalds; +Cc: open list:GPIO SUBSYSTEM, linux-kernel
Hi Linus,
last minute regressions galore. Here is a fixup for an
issue I found monday when testing unrelated patches.
Please pull it in!
Yours,
Linus Walleij
The following changes since commit 4b972a01a7da614b4796475f933094751a295a2f:
Linux 5.2-rc6 (2019-06-22 16:01:36 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
tags/gpio-v5.2-4
for you to fetch changes up to fbbf145a0e0a0177e089c52275fbfa55763e7d1d:
gpio/spi: Fix spi-gpio regression on active high CS (2019-07-02
22:31:37 +0200)
----------------------------------------------------------------
A single fixup for the SPI CS gpios that regressed
in the current kernel cycle.
----------------------------------------------------------------
Linus Walleij (1):
gpio/spi: Fix spi-gpio regression on active high CS
drivers/gpio/gpiolib-of.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
^ permalink raw reply
* Re: [RFC/RFT v3 11/14] arm64: dts: meson-g12a: add cpus OPP table
From: Martin Blumenstingl @ 2019-07-03 12:12 UTC (permalink / raw)
To: Neil Armstrong
Cc: jbrunet, khilman, linux-arm-kernel, linux-amlogic, linux-kernel,
linux-clk, linux-gpio
In-Reply-To: <7acdafc1-39e7-a2ec-886f-ca337c60dfe7@baylibre.com>
Hi Neil,
On Wed, Jul 3, 2019 at 1:53 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> On 03/07/2019 01:47, Martin Blumenstingl wrote:
> > Hi Neil,
> >
> > On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >>
> >> Add the OPP table taken from the vendor u200 and u211 DTS.
> >>
> >> The Amlogic G12A SoC seems to available in 3 types :
> >> - low-speed: up to 1,8GHz
> >> - mid-speed: up to 1,908GHz
> >> - high-speed: up to 2.1GHz
> >>
> >> And the S905X2 opp voltages are slightly higher than the S905D2
> >> OPP voltages for the low-speed table.
> >>
> >> This adds the conservative OPP table with the S905X2 higher voltages
> >> and the maximum low-speed OPP frequency.
> > have you considered all three as separate voltage tables?
> > you're other patches are assigning the OPP table to the CPU in the
> > board.dts anyways, so it's easy to use different OPP tables for
> > different boards
>
> We can't assume the board and the CPU type :-/
OK, should we assign the OPP table to the CPU cores then in the
soc.dtsi (instead of board.dts like the other patches from this series
do)?
> Kevin told me about cpufreq policy, where we could add a policy reading the
> eFUSE and changing the max frequency, then we could add the whole OPP table.
we can still do that in a second step, so I'm all for starting with
the "conservative" OPP table and then improve performance (by having
detecting the SoC and using the correct OPP table)
Martin
^ permalink raw reply
* Re: [RFC/RFT v3 14/14] arm64: dts: meson-g12b-odroid-n2: enable DVFS
From: Neil Armstrong @ 2019-07-03 11:54 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: jbrunet, khilman, linux-arm-kernel, linux-amlogic, linux-kernel,
linux-clk, linux-gpio
In-Reply-To: <CAFBinCA5-5kbD-0e7Lm7FwRneWsKeW4yQrucd1PAk=s2PrtxJQ@mail.gmail.com>
On 03/07/2019 01:45, Martin Blumenstingl wrote:
> On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> Enable DVFS for the Odroid-N2 by setting the clock, OPP and supply
>> for each cores of each CPU clusters.
>>
>> The first cluster uses the "VDDCPU_B" power supply, and the second
>> cluster uses the "VDDCPU_A" power supply.
>>
>> Each power supply can achieve 0.73V to 1.01V using 2 distinct PWM
>> outputs clocked at 800KHz with an inverse duty-cycle.
>>
>> DVFS has been tested by running the arm64 cpuburn at [1] and cycling
>> between all the possible cpufreq translations of each cluster and
>> checking the final frequency using the clock-measurer, script at [2].
>>
>> [1] https://github.com/ssvb/cpuburn-arm/blob/master/cpuburn-a53.S
>> [2] https://gist.github.com/superna9999/d4de964dbc0f84b7d527e1df2ddea25f
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> [...]
>> + vddcpu_b: regulator-vddcpu-b {
>> + /*
>> + * Silergy SY8120B1ABC Regulator.
>> + */
> interesting that they use different regulator ICs for CPU A and CPU B
> the public schematics confirm your comments
>
Yep they use a Silergy one for VDDCPU_B on every schematics I have.
The A311D VIM3 have a slightly different one, but still Silergy for VDDCPU_B.
https://dl.khadas.com/Hardware/VIM3/Schematic/VIM3_V11_Sch.pdf
Neil
^ permalink raw reply
* Re: [RFC/RFT v3 11/14] arm64: dts: meson-g12a: add cpus OPP table
From: Neil Armstrong @ 2019-07-03 11:53 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: jbrunet, khilman, linux-arm-kernel, linux-amlogic, linux-kernel,
linux-clk, linux-gpio
In-Reply-To: <CAFBinCBg57gh1x3CKs-YrCvTD0WR2s5zVGWtycb=RGqMiQ-VgA@mail.gmail.com>
On 03/07/2019 01:47, Martin Blumenstingl wrote:
> Hi Neil,
>
> On Mon, Jul 1, 2019 at 11:13 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> Add the OPP table taken from the vendor u200 and u211 DTS.
>>
>> The Amlogic G12A SoC seems to available in 3 types :
>> - low-speed: up to 1,8GHz
>> - mid-speed: up to 1,908GHz
>> - high-speed: up to 2.1GHz
>>
>> And the S905X2 opp voltages are slightly higher than the S905D2
>> OPP voltages for the low-speed table.
>>
>> This adds the conservative OPP table with the S905X2 higher voltages
>> and the maximum low-speed OPP frequency.
> have you considered all three as separate voltage tables?
> you're other patches are assigning the OPP table to the CPU in the
> board.dts anyways, so it's easy to use different OPP tables for
> different boards
We can't assume the board and the CPU type :-/
Kevin told me about cpufreq policy, where we could add a policy reading the
eFUSE and changing the max frequency, then we could add the whole OPP table.
Neil
>
^ 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