| Message ID | cover.1676042188.git.asmaa@nvidia.com |
|---|---|
| Headers | show |
| Series | Add NVIDIA BlueField-3 GPIO driver and pin controller | expand |
On Fri, Feb 10, 2023 at 10:39:40AM -0500, Asmaa Mnebhi wrote: > This patch adds support for the BlueField-3 SoC GPIO driver The Submitting Patches states that imperative speech should be used. > which allows: > - setting certain GPIOs as interrupts from other dependent drivers > - ability to manipulate certain GPIO pins via libgpiod tools > > BlueField-3 has 56 GPIOs but the user is only allowed to change some > of them into GPIO mode. Use valid_mask to make it impossible to alter > the rest of the GPIOs. ... > + help > + Say Y here if you want GPIO support on Mellanox BlueField 3 SoC. Have you run checkpatch? Nowadays 3+ lines of help is recommended. I would suggest to add a standard phrase about module name in case the module build is chosen. ... > +// SPDX-License-Identifier: GPL-2.0-only or BSD-3-Clause > + Redundant blank line > +/* > + * Copyright (C) 2022 NVIDIA CORPORATION & AFFILIATES > + */ This can be on one line. ... > +#include <linux/acpi.h> No user of this header. > +#include <linux/gpio/driver.h> > +#include <linux/platform_device.h> Approx dozen of header inclusions are missing. (bits.h, types.h, spinlock.h, ...) ... > +struct mlxbf3_gpio_context { > + struct gpio_chip gc; > + struct irq_chip irq_chip; Have you run it on v6.1+ kernels? This should not be here, i.e. it must be static and const. > + /* YU GPIO block address */ > + void __iomem *gpio_io; > + > + /* YU GPIO cause block address */ > + void __iomem *gpio_cause_io; > + > + /* Mask of valid gpios that can be accessed by software */ > + unsigned int valid_mask; > +}; ... > + generic_handle_irq(irq_find_mapping(gc->irq.domain, level)); There is a helper that unites these two calls together. ... > + bool fall = false; > + bool rise = false; Instead, just assign each of the in the corresponding switch-case. > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_EDGE_BOTH: > + fall = true; > + rise = true; > + break; > + case IRQ_TYPE_EDGE_RISING: > + rise = true; > + break; > + case IRQ_TYPE_EDGE_FALLING: > + fall = true; > + break; > + default: > + return -EINVAL; > + } ... > + raw_spin_lock_irqsave(&gs->gc.bgpio_lock, flags); > + if (fall) { > + val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN); > + val |= BIT(offset); > + writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_FALL_EN); > + } > + > + if (rise) { > + val = readl(gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN); > + val |= BIT(offset); > + writel(val, gs->gpio_io + MLXBF_GPIO_CAUSE_RISE_EN); > + } > + raw_spin_unlock_irqrestore(&gs->gc.bgpio_lock, flags); Don't you need to choose and lock the IRQ handler here? > +} ... > +static int mlxbf3_gpio_init_valid_mask(struct gpio_chip *gc, > + unsigned long *valid_mask, > + unsigned int ngpios) > +{ > + struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc); > + > + *valid_mask = gs->valid_mask; > + > + return 0; > +} Why do you need this? > + struct resource *res; Useless variable, see below. ... > + const char *name; > + name = dev_name(dev); Useless, just call dev_name() where it's needed. ... > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + /* Resource shared with pinctrl driver */ > + gs->gpio_io = devm_ioremap(dev, res->start, resource_size(res)); > + if (!gs->gpio_io) > + return -ENOMEM; > + > + /* YU GPIO block address */ > + gs->gpio_cause_io = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(gs->gpio_cause_io)) > + return PTR_ERR(gs->gpio_cause_io); These can be folded in a single devm_platform_ioremap_resource() call. ... > + if (device_property_read_u32(dev, "npins", &npins)) > + npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK; You can get of conditional. npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK; device_property_read_u32(dev, "npins", &npins); ... > + if (device_property_read_u32(dev, "valid_mask", &valid_mask)) > + valid_mask = 0x0; Besides that you can move this directly to the respective call and drop redundant private copy of valid mask, you mean that without that property no valid GPIOs? > + gs->valid_mask = valid_mask; ... > + girq->handler = handle_simple_irq; Should be handle_bad_irq() to avoid some subtle issues that hard to debug. ... > + ret = devm_request_irq(dev, irq, mlxbf3_gpio_irq_handler, > + IRQF_SHARED, name, gs); > + if (ret) { > + dev_err(dev, "failed to request IRQ"); > + return ret; return dev_err_probe(...); > + } > + } ... > + ret = devm_gpiochip_add_data(dev, &gs->gc, gs); > + if (ret) { > + dev_err(dev, "Failed adding memory mapped gpiochip\n"); > + return ret; Ditto. > + } ... > +static const struct acpi_device_id __maybe_unused mlxbf3_gpio_acpi_match[] = { > + { "MLNXBF33", 0 }, > + {}, No comma for termination entry. > +}; ... > + .probe = mlxbf3_gpio_probe, > +}; > + Redundant blank line. > +module_platform_driver(mlxbf3_gpio_driver);
On Fri, Feb 10, 2023 at 10:39:39AM -0500, Asmaa Mnebhi wrote: > This series of patches creates a pin controller driver and GPIO > driver for NVIDIA BlueField-3 SoC. > The first patch creates a GPIO driver for handling interrupts and > allowing the change of direction and value of a GPIO if needed. > The second patch creates a pin controller driver for allowing a > select number of GPIO pins to be manipulated from userspace or > the kernel. > > The BlueField-3 SoC gpio-mlxbf3.c driver handles different hardware registers > and logic that from gpio-mlxbf.c and gpio-mlxbf2.c. > For that reason, we have separate drivers for each generation. It seems you neglected to include maintainers and reviewers of the previous version(s) of your series. Don't do this. Please, respect people who invested their time in your code. Hint: I have a "smart" script [1] which helps to collect proper people (except reviewers that you need to add manually via --cc command line parameter) and mailing lists. Fell free to re-use, modify, send feedback. [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
Thanks Andy for the feedback! Will address your comment in v4. > + generic_handle_irq(irq_find_mapping(gc->irq.domain, level)); There is a helper that unites these two calls together. I didn't find any helper in v6.2. Could you please point me to it? > +static int mlxbf3_gpio_init_valid_mask(struct gpio_chip *gc, > + unsigned long *valid_mask, > + unsigned int ngpios) > +{ > + struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc); > + > + *valid_mask = gs->valid_mask; > + > + return 0; > +} Why do you need this? Since we only use ACPI tables and we want user space (libgpiod tool) or possibly (in the future) other kernel drivers to have access to certain GPIO pins, we use this valid_mask to do so. Linus previously explained that if we ask for a GPIO then it will be muxed in using .gpio_request_enable(). And so we would use .valid_mask to restrict the use of certain gpios.
Mon, Feb 13, 2023 at 11:14:06PM +0000, Asmaa Mnebhi kirjoitti: > Thanks Andy for the feedback! Will address your comment in v4. > > > + generic_handle_irq(irq_find_mapping(gc->irq.domain, level)); > > There is a helper that unites these two calls together. > > I didn't find any helper in v6.2. Could you please point me to it? It's available even longer than just in v6.2: generic_handle_domain_irq(). ... > > +static int mlxbf3_gpio_init_valid_mask(struct gpio_chip *gc, > > + unsigned long *valid_mask, > > + unsigned int ngpios) > > +{ > > + struct mlxbf3_gpio_context *gs = gpiochip_get_data(gc); > > + > > + *valid_mask = gs->valid_mask; > > + > > + return 0; > > +} > > Why do you need this? > > Since we only use ACPI tables and we want user space (libgpiod tool) or possibly (in the future) > other kernel drivers to have access to certain GPIO pins, we use this valid_mask to do so. > Linus previously explained that if we ask for a GPIO then it will be muxed in using .gpio_request_enable(). > And so we would use .valid_mask to restrict the use of certain gpios. So, why you can't use gpio-reserved-ranges property?
Support the BlueField-3 SoC GPIO driver for handling interrupts and providing the option to change the direction and value of a GPIO. Support the BlueField-3 SoC pin controller driver for allowing a select number of GPIO pins to be manipulated from userspace or the kernel. The gpio-mlxbf3.c driver handles hardware registers and logic that are different from gpio-mlxbf.c and gpio-mlxbf2.c. For that reason, we have separate drivers for each generation. Changes from v3->v4: gpio-mlxbf3.c: - Update the Kconfig file so that it is conform with checkpatch - Remove unncessary headers and add missing header inclusions - Make irq_chip struct static and const - Replace generic_handle_irq(irq_find_mapping) with generic_handle_domain_irq - Simplify logic in irq_set_type - Replace valid_mask with gpio-reserved-ranges - Cleanup code pinctrl-mlxbf.c: - Cleanup code - Update the Kconfig file so that it is conform with checkpatch - Remove unncessary headers and add missing header inclusions Asmaa Mnebhi (2): gpio: gpio-mlxbf3: Add gpio driver support pinctrl: pinctrl-mlxbf: Add pinctrl driver support drivers/gpio/Kconfig | 12 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-mlxbf3.c | 230 ++++++++++++++++++++++ drivers/pinctrl/Kconfig | 14 ++ drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-mlxbf.c | 338 ++++++++++++++++++++++++++++++++ 6 files changed, 596 insertions(+) create mode 100644 drivers/gpio/gpio-mlxbf3.c create mode 100644 drivers/pinctrl/pinctrl-mlxbf.c
On Fri, Feb 17, 2023 at 11:27 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote: > > NVIDIA BlueField-3 SoC has a few pins that can be used as GPIOs > or take the default hardware functionality. Add a driver for > the pinmuxing. pin muxing ... > +++ b/drivers/pinctrl/pinctrl-mlxbf.c Wondering if it would be better to match the GPIO driver naming schema, i.e. by adding 3. In this case the additional explanation in Kconfig help won't be necessary. ... > +#define MLXBF_GPIO0_FW_CONTROL_SET 0 > +#define MLXBF_GPIO0_FW_CONTROL_CLEAR 0x14 > +#define MLXBF_GPIO1_FW_CONTROL_SET 0x80 > +#define MLXBF_GPIO1_FW_CONTROL_CLEAR 0x94 Unclear if these are commands or register offsets. If they are of the same type (semantically), make them fixed width, e.g., 0x00. ... > +enum { > + MLXBF_GPIO_HW_MODE, > + MLXBF_GPIO_SW_MODE I would leave a comma here as it might be extended in the future. > +}; ... > +static const char * const mlxbf_pinctrl_single_group_names[] = { > + "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", > + "gpio7", "gpio8", "gpio9", "gpio10", "gpio11", "gpio12", "gpio13", > + "gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20", > + "gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27", > + "gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34", > + "gpio35", "gpio36", "gpio37", "gpio38", "gpio39", "gpio40", "gpio41", > + "gpio42", "gpio43", "gpio44", "gpio45", "gpio46", "gpio47", "gpio48", > + "gpio49", "gpio50", "gpio51", "gpio52", "gpio53", "gpio54", "gpio55" Ditto. Can you group by 8? > +}; > + > +/* Set of pin numbers for single-pin groups */ > +static const unsigned int mlxbf_pinctrl_single_group_pins[] = { > + 0, 1, 2, 3, 4, 5, 6, > + 7, 8, 9, 10, 11, 12, 13, > + 14, 15, 16, 17, 18, 19, 20, > + 21, 22, 23, 24, 25, 26, 27, > + 28, 29, 30, 31, 32, 33, 34, > + 35, 36, 37, 38, 39, 40, 41, > + 42, 43, 44, 45, 46, 47, 48, > + 49, 50, 51, 52, 53, 54, 55, Group by 8 which is the more natural length of subarray per line. Is it just 1:1 to the index? If so, why do you need this table at all? > +}; ... > +static const struct { > + const char *name; > + const char * const *group_names; Use this instead https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215 and this https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222 > +} mlxbf_pmx_funcs[] = { > +}; ... > +{ > + struct mlxbf_pinctrl *priv = pinctrl_dev_get_drvdata(pctldev); > + > + /* disable GPIO functionality by giving control back to hardware */ > + if (offset < MLXBF_NGPIOS_GPIO0) > + writel(BIT(offset), priv->base + MLXBF_GPIO0_FW_CONTROL_CLEAR); > + else > + writel(BIT(offset % MLXBF_NGPIOS_GPIO0), priv->base + MLXBF_GPIO1_FW_CONTROL_CLEAR); > + Redundant blank line. > +} ... > +static_assert(ARRAY_SIZE(mlxbf_pinctrl_single_group_names) == > + ARRAY_SIZE(mlxbf_pinctrl_single_group_pins)); I would put it on a single line, but it's up to you. ... > + struct resource *res; Useless? ... > + /* This resource is shared so use devm_ioremap */ Can you elaborate on who actually requests the region? And why is it not _this_ driver? > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; ... > + ret = devm_pinctrl_register_and_init(priv->dev, Is the priv->dev different from dev? > + &mlxbf_pin_desc, > + priv, > + &priv->pctl); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to register pinctrl\n"); ... > + pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[0]); > + pinctrl_add_gpio_range(priv->pctl, &mlxbf_pinctrl_gpio_ranges[1]); pinctrl_add_gpio_ranges() ? -- With Best Regards, Andy Shevchenko
> > +static const struct { > > + const char *name; > > + const char * const *group_names; > > Use this instead > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux- > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215 > and this > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux- > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222 > > > +} mlxbf_pmx_funcs[] = { > > > +}; so copy that struct definition and macro to my driver? (I don’t see these code changes in master) > > > + /* This resource is shared so use devm_ioremap */ > > Can you elaborate on who actually requests the region? And why is it not > _this_ driver? This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it accesses several other registers offsets in between.
On Thu, Feb 23, 2023 at 11:07 PM Asmaa Mnebhi <asmaa@nvidia.com> wrote: > > > > +static const struct { > > > + const char *name; > > > + const char * const *group_names; > > > > Use this instead > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux- > > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n215 > > and this > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux- > > pinctrl.git/tree/include/linux/pinctrl/pinctrl.h?h=devel#n222 > > > > > +} mlxbf_pmx_funcs[] = { > > > > > +}; > > so copy that struct definition and macro to my driver? (I don’t see these code changes in master) Which master? First of all, you should do your development based on the "for-next" of the respective subsystem (okay, for pin control Linus Walleij called his published branch "devel"). So, the above mentioned functionality was there a while ago. Second, a couple of days ago Linus Torvalds pulled PR, so it's part of upstream now. TL;DR: just use those types and macros in your code. > > > + /* This resource is shared so use devm_ioremap */ > > > > Can you elaborate on who actually requests the region? And why is it not > > _this_ driver? > > This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it accesses several other registers offsets in between. Okay, so in such a case you need a common denominator that actually does this all for you via, for example, regmap. If the region is not requested, bad things may happen.
> > > > + /* This resource is shared so use devm_ioremap */ > > > > > > Can you elaborate on who actually requests the region? And why is it > > > not _this_ driver? > > > > This resource is shared with the gpio-mlxbf3.c driver. The gpio-mlxbf3.c > driver does not access the same offsets as the pinctrl-mlxbf3.c driver, but it > accesses several other registers offsets in between. > > Okay, so in such a case you need a common denominator that actually does > this all for you via, for example, regmap. If the region is not requested, bad > things may happen. Hi Andy, I have taken a look at the regmap driver and decided we will maintain the Current implementation with devm_platform_ioremap_resource. The main reason is for backward compatibility with older kernels. Although we Do support the latest kernel, we have some customers who are still using old Kernels which don’t have regmap support. But I have found a solution which Involves using devm_platform_ioremap_resource here as you initially suggested. patch v5 coming soon! Thanks. Asmaa