Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
(System Integration Unit Lite2) hardware module. There are two
SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
SIUL2_1 for the rest.
The GPIOs are not fully contiguous, there are some gaps:
- GPIO102 up to GPIO111(inclusive) are invalid
- GPIO123 up to GPIO143(inclusive) are invalid
Some GPIOs are input only(i.e. GPI182) though this restriction
is not yet enforced in code.
This patch adds basic GPIO functionality(no interrupts, no
suspend/resume functions).
Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
drivers/gpio/Kconfig | 10 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-siul2-s32g2.c | 576 ++++++++++++++++++++++++++++++++
3 files changed, 587 insertions(+)
create mode 100644 drivers/gpio/gpio-siul2-s32g2.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d93cd4f722b4..ae6aa6f64db3 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -643,6 +643,16 @@ config GPIO_SIOX
Say yes here to support SIOX I/O devices. These are units connected
via a SIOX bus and have a number of fixed-direction I/O lines.
+config GPIO_SIUL2_S32G2
+ tristate "GPIO driver for S32G2/S32G3"
+ depends on ARCH_S32 || COMPILE_TEST
+ depends on OF_GPIO
+ select REGMAP_MMIO
+ help
+ This enables support for the SIUL2 GPIOs found on the S32G2/S32G3
+ chips. Say yes here to enable the SIUL2 to be used as an GPIO
+ controller for S32G2/S32G3 platforms.
+
config GPIO_SNPS_CREG
bool "Synopsys GPIO via CREG (Control REGisters) driver"
depends on ARC || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1429e8c0229b..8d5f35689fed 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o
obj-$(CONFIG_GPIO_SIM) += gpio-sim.o
obj-$(CONFIG_GPIO_SIOX) += gpio-siox.o
+obj-$(CONFIG_GPIO_SIUL2_S32G2) += gpio-siul2-s32g2.o
obj-$(CONFIG_GPIO_SL28CPLD) += gpio-sl28cpld.o
obj-$(CONFIG_GPIO_SLOPPY_LOGIC_ANALYZER) += gpio-sloppy-logic-analyzer.o
obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o
diff --git a/drivers/gpio/gpio-siul2-s32g2.c b/drivers/gpio/gpio-siul2-s32g2.c
new file mode 100644
index 000000000000..d9c04aacb3cc
--- /dev/null
+++ b/drivers/gpio/gpio-siul2-s32g2.c
@@ -0,0 +1,576 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * SIUL2 GPIO support.
+ *
+ * Copyright (c) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2018-2024 NXP
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/gpio/driver.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+/* PGPDOs are 16bit registers that come in big endian
+ * order if they are grouped in pairs of two.
+ *
+ * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2...
+ */
+#define SIUL2_PGPDO(N) (((N) ^ 1) * 2)
+#define S32G2_SIUL2_NUM 2
+#define S32G2_PADS_DTS_TAG_LEN 7
+
+#define SIUL2_GPIO_16_PAD_SIZE 16
+
+/**
+ * struct siul2_device_data - platform data attached to the compatible.
+ * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
+ * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
+ */
+struct siul2_device_data {
+ const struct regmap_access_table **pad_access;
+ const bool reset_cnt;
+};
+
+/**
+ * struct siul2_desc - describes a SIUL2 hw module.
+ * @pad_access: array of valid I/O pads.
+ * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
+ * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
+ * @gpio_base: the first GPIO pin.
+ * @gpio_num: the number of GPIO pins.
+ */
+struct siul2_desc {
+ const struct regmap_access_table *pad_access;
+ struct regmap *opadmap;
+ struct regmap *ipadmap;
+ u32 gpio_base;
+ u32 gpio_num;
+};
+
+/**
+ * struct siul2_gpio_dev - describes a group of GPIO pins.
+ * @platdata: the platform data.
+ * @siul2: SIUL2_0 and SIUL2_1 modules information.
+ * @pin_dir_bitmap: the bitmap with pin directions.
+ * @gc: the GPIO chip.
+ * @lock: mutual access to bitmaps.
+ */
+struct siul2_gpio_dev {
+ const struct siul2_device_data *platdata;
+ struct siul2_desc siul2[S32G2_SIUL2_NUM];
+ unsigned long *pin_dir_bitmap;
+ struct gpio_chip gc;
+ raw_spinlock_t lock;
+};
+
+static const struct regmap_range s32g2_siul20_pad_yes_ranges[] = {
+ regmap_reg_range(SIUL2_PGPDO(0), SIUL2_PGPDO(0)),
+ regmap_reg_range(SIUL2_PGPDO(1), SIUL2_PGPDO(1)),
+ regmap_reg_range(SIUL2_PGPDO(2), SIUL2_PGPDO(2)),
+ regmap_reg_range(SIUL2_PGPDO(3), SIUL2_PGPDO(3)),
+ regmap_reg_range(SIUL2_PGPDO(4), SIUL2_PGPDO(4)),
+ regmap_reg_range(SIUL2_PGPDO(5), SIUL2_PGPDO(5)),
+ regmap_reg_range(SIUL2_PGPDO(6), SIUL2_PGPDO(6)),
+};
+
+static const struct regmap_access_table s32g2_siul20_pad_access_table = {
+ .yes_ranges = s32g2_siul20_pad_yes_ranges,
+ .n_yes_ranges = ARRAY_SIZE(s32g2_siul20_pad_yes_ranges),
+};
+
+static const struct regmap_range s32g2_siul21_pad_yes_ranges[] = {
+ regmap_reg_range(SIUL2_PGPDO(7), SIUL2_PGPDO(7)),
+ regmap_reg_range(SIUL2_PGPDO(9), SIUL2_PGPDO(9)),
+ regmap_reg_range(SIUL2_PGPDO(10), SIUL2_PGPDO(10)),
+ regmap_reg_range(SIUL2_PGPDO(11), SIUL2_PGPDO(11)),
+};
+
+static const struct regmap_access_table s32g2_siul21_pad_access_table = {
+ .yes_ranges = s32g2_siul21_pad_yes_ranges,
+ .n_yes_ranges = ARRAY_SIZE(s32g2_siul21_pad_yes_ranges),
+};
+
+static const struct regmap_access_table *s32g2_pad_access_table[] = {
+ &s32g2_siul20_pad_access_table,
+ &s32g2_siul21_pad_access_table
+};
+
+static_assert(ARRAY_SIZE(s32g2_pad_access_table) == S32G2_SIUL2_NUM);
+
+static const struct siul2_device_data s32g2_device_data = {
+ .pad_access = s32g2_pad_access_table,
+ .reset_cnt = true,
+};
+
+static int siul2_get_gpio_pinspec(struct platform_device *pdev,
+ struct of_phandle_args *pinspec,
+ unsigned int range_index)
+{
+ struct device_node *np = pdev->dev.of_node;
+
+ return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
+ range_index, pinspec);
+}
+
+static struct regmap *siul2_offset_to_regmap(struct siul2_gpio_dev *dev,
+ unsigned int offset,
+ bool input)
+{
+ struct siul2_desc *siul2;
+ size_t i;
+
+ for (i = 0; i < ARRAY_SIZE(dev->siul2); i++) {
+ siul2 = &dev->siul2[i];
+ if (offset >= siul2->gpio_base &&
+ offset - siul2->gpio_base < siul2->gpio_num)
+ return input ? siul2->ipadmap : siul2->opadmap;
+ }
+
+ return NULL;
+}
+
+static void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
+ unsigned int gpio, int dir)
+{
+ guard(raw_spinlock_irqsave)(&dev->lock);
+
+ if (dir == GPIO_LINE_DIRECTION_IN)
+ __clear_bit(gpio, dev->pin_dir_bitmap);
+ else
+ __set_bit(gpio, dev->pin_dir_bitmap);
+}
+
+static int siul2_get_direction(struct siul2_gpio_dev *dev,
+ unsigned int gpio)
+{
+ return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
+ GPIO_LINE_DIRECTION_IN;
+}
+
+static struct siul2_gpio_dev *to_siul2_gpio_dev(struct gpio_chip *chip)
+{
+ return container_of(chip, struct siul2_gpio_dev, gc);
+}
+
+static int siul2_gpio_dir_in(struct gpio_chip *chip, unsigned int gpio)
+{
+ struct siul2_gpio_dev *gpio_dev;
+ int ret = 0;
+
+ ret = pinctrl_gpio_direction_input(chip, gpio);
+ if (ret)
+ return ret;
+
+ gpio_dev = to_siul2_gpio_dev(chip);
+ siul2_gpio_set_direction(gpio_dev, gpio, GPIO_LINE_DIRECTION_IN);
+
+ return 0;
+}
+
+static int siul2_gpio_get_dir(struct gpio_chip *chip, unsigned int gpio)
+{
+ return siul2_get_direction(to_siul2_gpio_dev(chip), gpio);
+}
+
+static unsigned int siul2_pin2pad(unsigned int pin)
+{
+ return pin / SIUL2_GPIO_16_PAD_SIZE;
+}
+
+static u16 siul2_pin2mask(unsigned int pin)
+{
+ /**
+ * From Reference manual :
+ * PGPDOx[PPDOy] = GPDO(x × 16) + (15 - y)[PDO_(x × 16) + (15 - y)]
+ */
+ return BIT(SIUL2_GPIO_16_PAD_SIZE - 1 - pin % SIUL2_GPIO_16_PAD_SIZE);
+}
+
+static void siul2_gpio_set_val(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+ unsigned int pad, reg_offset;
+ struct regmap *regmap;
+ u16 mask;
+
+ mask = siul2_pin2mask(offset);
+ pad = siul2_pin2pad(offset);
+
+ reg_offset = SIUL2_PGPDO(pad);
+ regmap = siul2_offset_to_regmap(gpio_dev, offset, false);
+ if (!regmap)
+ return;
+
+ value = value ? mask : 0;
+
+ regmap_update_bits(regmap, reg_offset, mask, value);
+}
+
+static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
+ int val)
+{
+ struct siul2_gpio_dev *gpio_dev;
+ int ret = 0;
+
+ gpio_dev = to_siul2_gpio_dev(chip);
+ siul2_gpio_set_val(chip, gpio, val);
+
+ ret = pinctrl_gpio_direction_output(chip, gpio);
+ if (ret)
+ return ret;
+
+ siul2_gpio_set_direction(gpio_dev, gpio, GPIO_LINE_DIRECTION_OUT);
+
+ return 0;
+}
+
+static void siul2_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+
+ if (!gpio_dev)
+ return;
+
+ if (siul2_get_direction(gpio_dev, offset) == GPIO_LINE_DIRECTION_IN)
+ return;
+
+ siul2_gpio_set_val(chip, offset, value);
+}
+
+static int siul2_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+ unsigned int mask, pad, reg_offset, data = 0;
+ struct regmap *regmap;
+
+ mask = siul2_pin2mask(offset);
+ pad = siul2_pin2pad(offset);
+
+ reg_offset = SIUL2_PGPDO(pad);
+ regmap = siul2_offset_to_regmap(gpio_dev, offset, true);
+ if (!regmap)
+ return -EINVAL;
+
+ regmap_read(regmap, reg_offset, &data);
+
+ return !!(data & mask);
+}
+
+static const struct regmap_config siul2_regmap_conf = {
+ .val_bits = 32,
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .cache_type = REGCACHE_FLAT,
+};
+
+static struct regmap *common_regmap_init(struct platform_device *pdev,
+ struct regmap_config *conf,
+ const char *name)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ resource_size_t size;
+ void __iomem *base;
+
+ base = devm_platform_get_and_ioremap_resource_byname(pdev, name, &res);
+ if (IS_ERR(base)) {
+ dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
+ return ERR_PTR(-EINVAL);
+ }
+
+ size = resource_size(res);
+ conf->val_bits = conf->reg_stride * 8;
+ conf->max_register = size - conf->reg_stride;
+ conf->name = name;
+ conf->use_raw_spinlock = true;
+
+ if (conf->cache_type != REGCACHE_NONE)
+ conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
+
+ return devm_regmap_init_mmio(dev, base, conf);
+}
+
+static bool not_writable(__always_unused struct device *dev,
+ __always_unused unsigned int reg)
+{
+ return false;
+}
+
+static struct regmap *init_padregmap(struct platform_device *pdev,
+ struct siul2_gpio_dev *gpio_dev,
+ int selector, bool input)
+{
+ const struct siul2_device_data *platdata = gpio_dev->platdata;
+ struct regmap_config regmap_conf = siul2_regmap_conf;
+ char dts_tag[S32G2_PADS_DTS_TAG_LEN];
+ int err;
+
+ regmap_conf.reg_stride = 2;
+
+ if (selector != 0 && selector != 1)
+ return ERR_PTR(-EINVAL);
+
+ regmap_conf.rd_table = platdata->pad_access[selector];
+
+ err = snprintf(dts_tag, ARRAY_SIZE(dts_tag), "%cpads%d",
+ input ? 'i' : 'o', selector);
+ if (err < 0)
+ return ERR_PTR(-EINVAL);
+
+ if (input) {
+ regmap_conf.writeable_reg = not_writable;
+ regmap_conf.cache_type = REGCACHE_NONE;
+ } else {
+ regmap_conf.wr_table = platdata->pad_access[selector];
+ }
+
+ return common_regmap_init(pdev, ®map_conf, dts_tag);
+}
+
+static int siul2_gpio_pads_init(struct platform_device *pdev,
+ struct siul2_gpio_dev *gpio_dev)
+{
+ struct device *dev = &pdev->dev;
+ size_t i;
+
+ for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
+ gpio_dev->siul2[i].opadmap = init_padregmap(pdev, gpio_dev, i,
+ false);
+ if (IS_ERR(gpio_dev->siul2[i].opadmap)) {
+ dev_err(dev,
+ "Failed to initialize opad2%zu regmap config\n",
+ i);
+ return PTR_ERR(gpio_dev->siul2[i].opadmap);
+ }
+
+ gpio_dev->siul2[i].ipadmap = init_padregmap(pdev, gpio_dev, i,
+ true);
+ if (IS_ERR(gpio_dev->siul2[i].ipadmap)) {
+ dev_err(dev,
+ "Failed to initialize ipad2%zu regmap config\n",
+ i);
+ return PTR_ERR(gpio_dev->siul2[i].ipadmap);
+ }
+ }
+
+ return 0;
+}
+
+static int siul2_gen_names(struct device *dev, unsigned int cnt, char **names,
+ char *ch_index, unsigned int *num_index)
+{
+ unsigned int i;
+
+ for (i = 0; i < cnt; i++) {
+ if (i != 0 && !(*num_index % 16))
+ (*ch_index)++;
+
+ names[i] = devm_kasprintf(dev, GFP_KERNEL, "P%c_%02d",
+ *ch_index, 0xFU & (*num_index)++);
+ if (!names[i])
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static int siul2_gpio_remove_reserved_names(struct device *dev,
+ struct siul2_gpio_dev *gpio_dev,
+ char **names)
+{
+ struct device_node *np = dev->of_node;
+ int num_ranges, i, j, ret;
+ u32 base_gpio, num_gpio;
+
+ /* Parse the gpio-reserved-ranges to know which GPIOs to exclude. */
+
+ num_ranges = of_property_count_u32_elems(dev->of_node,
+ "gpio-reserved-ranges");
+
+ /* The "gpio-reserved-ranges" is optional. */
+ if (num_ranges < 0)
+ return 0;
+ num_ranges /= 2;
+
+ for (i = 0; i < num_ranges; i++) {
+ ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
+ i * 2, &base_gpio);
+ if (ret) {
+ dev_err(dev, "Could not parse the start GPIO: %d\n",
+ ret);
+ return ret;
+ }
+
+ ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
+ i * 2 + 1, &num_gpio);
+ if (ret) {
+ dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
+ return ret;
+ }
+
+ if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
+ dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
+ return -EINVAL;
+ }
+
+ /* Remove names set for reserved GPIOs. */
+ for (j = base_gpio; j < base_gpio + num_gpio; j++) {
+ devm_kfree(dev, names[j]);
+ names[j] = NULL;
+ }
+ }
+
+ return 0;
+}
+
+static int siul2_gpio_populate_names(struct device *dev,
+ struct siul2_gpio_dev *gpio_dev)
+{
+ unsigned int num_index = 0;
+ char ch_index = 'A';
+ char **names;
+ int i, ret;
+
+ names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
+ GFP_KERNEL);
+ if (!names)
+ return -ENOMEM;
+
+ for (i = 0; i < S32G2_SIUL2_NUM; i++) {
+ ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
+ names + gpio_dev->siul2[i].gpio_base,
+ &ch_index, &num_index);
+ if (ret) {
+ dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
+ i);
+ return ret;
+ }
+
+ if (gpio_dev->platdata->reset_cnt)
+ num_index = 0;
+
+ ch_index++;
+ }
+
+ ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
+ if (ret)
+ return ret;
+
+ gpio_dev->gc.names = (const char *const *)names;
+
+ return 0;
+}
+
+static int siul2_gpio_probe(struct platform_device *pdev)
+{
+ struct siul2_gpio_dev *gpio_dev;
+ struct device *dev = &pdev->dev;
+ struct of_phandle_args pinspec;
+ size_t i, bitmap_size;
+ struct gpio_chip *gc;
+ int ret = 0;
+
+ gpio_dev = devm_kzalloc(dev, sizeof(*gpio_dev), GFP_KERNEL);
+ if (!gpio_dev)
+ return -ENOMEM;
+
+ gpio_dev->platdata = &s32g2_device_data;
+
+ for (i = 0; i < S32G2_SIUL2_NUM; i++)
+ gpio_dev->siul2[i].pad_access =
+ gpio_dev->platdata->pad_access[i];
+
+ ret = siul2_gpio_pads_init(pdev, gpio_dev);
+ if (ret)
+ return ret;
+
+ gc = &gpio_dev->gc;
+
+ platform_set_drvdata(pdev, gpio_dev);
+
+ raw_spin_lock_init(&gpio_dev->lock);
+
+ for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
+ ret = siul2_get_gpio_pinspec(pdev, &pinspec, i);
+ if (ret) {
+ dev_err(dev,
+ "unable to get pinspec %zu from device tree\n",
+ i);
+ return -EINVAL;
+ }
+
+ of_node_put(pinspec.np);
+
+ if (pinspec.args_count != 3) {
+ dev_err(dev, "Invalid pinspec count: %d\n",
+ pinspec.args_count);
+ return -EINVAL;
+ }
+
+ gpio_dev->siul2[i].gpio_base = pinspec.args[1];
+ gpio_dev->siul2[i].gpio_num = pinspec.args[2];
+ }
+
+ gc->base = -1;
+
+ /* In some cases, there is a gap between the SIUL GPIOs. */
+ gc->ngpio = gpio_dev->siul2[S32G2_SIUL2_NUM - 1].gpio_base +
+ gpio_dev->siul2[S32G2_SIUL2_NUM - 1].gpio_num;
+
+ ret = siul2_gpio_populate_names(&pdev->dev, gpio_dev);
+ if (ret)
+ return ret;
+
+ bitmap_size = BITS_TO_LONGS(gc->ngpio) *
+ sizeof(*gpio_dev->pin_dir_bitmap);
+ gpio_dev->pin_dir_bitmap = devm_kzalloc(dev, bitmap_size, GFP_KERNEL);
+ if (!gpio_dev->pin_dir_bitmap)
+ return -ENOMEM;
+
+ gc->parent = dev;
+ gc->label = dev_name(dev);
+
+ gc->set = siul2_gpio_set;
+ gc->get = siul2_gpio_get;
+ gc->set_config = gpiochip_generic_config;
+ gc->request = gpiochip_generic_request;
+ gc->free = gpiochip_generic_free;
+ gc->direction_output = siul2_gpio_dir_out;
+ gc->direction_input = siul2_gpio_dir_in;
+ gc->get_direction = siul2_gpio_get_dir;
+ gc->owner = THIS_MODULE;
+
+ ret = devm_gpiochip_add_data(dev, gc, gpio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "unable to add gpiochip\n");
+
+ return 0;
+}
+
+static const struct of_device_id siul2_gpio_dt_ids[] = {
+ { .compatible = "nxp,s32g2-siul2-gpio" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, siul2_gpio_dt_ids);
+
+static struct platform_driver siul2_gpio_driver = {
+ .driver = {
+ .name = "s32g2-siul2-gpio",
+ .of_match_table = siul2_gpio_dt_ids,
+ },
+ .probe = siul2_gpio_probe,
+};
+
+module_platform_driver(siul2_gpio_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP SIUL2 GPIO");
+MODULE_LICENSE("GPL");
--
2.45.2
Hi Andrei,
thanks for your patch!
Sorry for being so late in giving any deeper review of it.
On Thu, Sep 26, 2024 at 4:32 PM Andrei Stefanescu
<andrei.stefanescu@oss.nxp.com> wrote:
> Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
> (System Integration Unit Lite2) hardware module. There are two
> SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
> SIUL2_1 for the rest.
>
> The GPIOs are not fully contiguous, there are some gaps:
> - GPIO102 up to GPIO111(inclusive) are invalid
> - GPIO123 up to GPIO143(inclusive) are invalid
>
> Some GPIOs are input only(i.e. GPI182) though this restriction
> is not yet enforced in code.
>
> This patch adds basic GPIO functionality(no interrupts, no
> suspend/resume functions).
>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
(...)
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/pinctrl/consumer.h>
Hm, are you sure you don't want one of those combined
GPIO+pinctrl drivers? Look in drivers/pinctrl for examples such
as drivers/pinctrl/pinctrl-stmfx.c
> +/* PGPDOs are 16bit registers that come in big endian
> + * order if they are grouped in pairs of two.
> + *
> + * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2...
> + */
> +#define SIUL2_PGPDO(N) (((N) ^ 1) * 2)
> +#define S32G2_SIUL2_NUM 2
> +#define S32G2_PADS_DTS_TAG_LEN 7
> +
> +#define SIUL2_GPIO_16_PAD_SIZE 16
You seem to be manipulating "pads" which is pin control territory.
That should be done in a pin control driver.
> +/**
> + * struct siul2_device_data - platform data attached to the compatible.
> + * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
> + * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
> + */
> +struct siul2_device_data {
> + const struct regmap_access_table **pad_access;
> + const bool reset_cnt;
I don't understand the reset_cnt variable at all. Explain why it exists in the
kerneldoc please.
> +/**
> + * struct siul2_desc - describes a SIUL2 hw module.
> + * @pad_access: array of valid I/O pads.
Now that is really pin control isn't it.
> + * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
> + * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
> + * @gpio_base: the first GPIO pin.
> + * @gpio_num: the number of GPIO pins.
> + */
> +struct siul2_desc {
> + const struct regmap_access_table *pad_access;
> + struct regmap *opadmap;
> + struct regmap *ipadmap;
> + u32 gpio_base;
> + u32 gpio_num;
> +};
(...)
> +static int siul2_get_gpio_pinspec(struct platform_device *pdev,
> + struct of_phandle_args *pinspec,
> + unsigned int range_index)
> +{
> + struct device_node *np = pdev->dev.of_node;
> +
> + return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> + range_index, pinspec);
> +}
I do not see why a driver would parse gpio-ranges since the gpiolib
core should normally deal with the translation between GPIO lines
and pins.
This looks hacky and probably an indication that you want to use
a combined pinctrl+gpio driver so the different parts of the driver
can access the same structures easily without hacks.
> +static void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
> + unsigned int gpio, int dir)
> +{
> + guard(raw_spinlock_irqsave)(&dev->lock);
> +
> + if (dir == GPIO_LINE_DIRECTION_IN)
> + __clear_bit(gpio, dev->pin_dir_bitmap);
> + else
> + __set_bit(gpio, dev->pin_dir_bitmap);
> +}
This looks like a job for gpio regmap?
select GPIO_REGMAP,
look in other driver for examples of how to use it.
> +static int siul2_get_direction(struct siul2_gpio_dev *dev,
> + unsigned int gpio)
> +{
> + return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
> + GPIO_LINE_DIRECTION_IN;
> +}
Also looks like a reimplementation of GPIO_REGMAP.
> +static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
> + int val)
> +{
> + struct siul2_gpio_dev *gpio_dev;
> + int ret = 0;
> +
> + gpio_dev = to_siul2_gpio_dev(chip);
> + siul2_gpio_set_val(chip, gpio, val);
> +
> + ret = pinctrl_gpio_direction_output(chip, gpio);
(...)
This is nice, pin control is properly used as the back-end.
> +static int siul2_gpio_remove_reserved_names(struct device *dev,
> + struct siul2_gpio_dev *gpio_dev,
> + char **names)
> +{
> + struct device_node *np = dev->of_node;
> + int num_ranges, i, j, ret;
> + u32 base_gpio, num_gpio;
> +
> + /* Parse the gpio-reserved-ranges to know which GPIOs to exclude. */
> +
> + num_ranges = of_property_count_u32_elems(dev->of_node,
> + "gpio-reserved-ranges");
> +
> + /* The "gpio-reserved-ranges" is optional. */
> + if (num_ranges < 0)
> + return 0;
> + num_ranges /= 2;
> +
> + for (i = 0; i < num_ranges; i++) {
> + ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
> + i * 2, &base_gpio);
> + if (ret) {
> + dev_err(dev, "Could not parse the start GPIO: %d\n",
> + ret);
> + return ret;
> + }
> +
> + ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
> + i * 2 + 1, &num_gpio);
> + if (ret) {
> + dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
> + return ret;
> + }
> +
> + if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
> + dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
> + return -EINVAL;
> + }
> +
> + /* Remove names set for reserved GPIOs. */
> + for (j = base_gpio; j < base_gpio + num_gpio; j++) {
> + devm_kfree(dev, names[j]);
> + names[j] = NULL;
> + }
> + }
> +
> + return 0;
> +}
I don't get this. gpio-reserved-ranges is something that is parsed and
handled by gpiolib. Read the code in gpiolib.c and you'll probably
figure out how to let gpiolib take care of this for you.
> +static int siul2_gpio_populate_names(struct device *dev,
> + struct siul2_gpio_dev *gpio_dev)
> +{
> + unsigned int num_index = 0;
> + char ch_index = 'A';
> + char **names;
> + int i, ret;
> +
> + names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
> + GFP_KERNEL);
> + if (!names)
> + return -ENOMEM;
> +
> + for (i = 0; i < S32G2_SIUL2_NUM; i++) {
> + ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
> + names + gpio_dev->siul2[i].gpio_base,
> + &ch_index, &num_index);
> + if (ret) {
> + dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
> + i);
> + return ret;
> + }
> +
> + if (gpio_dev->platdata->reset_cnt)
> + num_index = 0;
> +
> + ch_index++;
> + }
> +
> + ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
> + if (ret)
> + return ret;
> +
> + gpio_dev->gc.names = (const char *const *)names;
> +
> + return 0;
> +}
Interesting!
I'm not against, on the contrary this looks really helpful to users.
> + gc = &gpio_dev->gc;
No poking around in the gpiolib internals please.
Look at what other drivers do and do like they do.
On top of these comments:
everywhere you do [raw_spin_]locks: try to use guards or
scoped guards instead. git grep guard drivers/gpio for
many examples.
Yours,
Linus Walleij
Hi Linus,
Thank you for the review!
On 01/10/2024 16:15, Linus Walleij wrote:
> Hi Andrei,
>
> thanks for your patch!
>
> Sorry for being so late in giving any deeper review of it.
>
> On Thu, Sep 26, 2024 at 4:32 PM Andrei Stefanescu
> <andrei.stefanescu@oss.nxp.com> wrote:
>
>> Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
>> (System Integration Unit Lite2) hardware module. There are two
>> SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
>> SIUL2_1 for the rest.
>>
>> The GPIOs are not fully contiguous, there are some gaps:
>> - GPIO102 up to GPIO111(inclusive) are invalid
>> - GPIO123 up to GPIO143(inclusive) are invalid
>>
>> Some GPIOs are input only(i.e. GPI182) though this restriction
>> is not yet enforced in code.
>>
>> This patch adds basic GPIO functionality(no interrupts, no
>> suspend/resume functions).
>>
>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
>
> (...)
I will add the Co-developed-by tags in v5.
>
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/pinctrl/consumer.h>
>
> Hm, are you sure you don't want one of those combined
> GPIO+pinctrl drivers? Look in drivers/pinctrl for examples such
> as drivers/pinctrl/pinctrl-stmfx.c
Thank you for the suggestion! I will merge the two drivers in v5.
>
>> +/* PGPDOs are 16bit registers that come in big endian
>> + * order if they are grouped in pairs of two.
>> + *
>> + * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2...
>> + */
>> +#define SIUL2_PGPDO(N) (((N) ^ 1) * 2)
>> +#define S32G2_SIUL2_NUM 2
>> +#define S32G2_PADS_DTS_TAG_LEN 7
>> +
>> +#define SIUL2_GPIO_16_PAD_SIZE 16
>
> You seem to be manipulating "pads" which is pin control territory.
> That should be done in a pin control driver.
>
This is more of a case of bad naming, the registers used for
setting/reading a GPIO's value are called Parallel GPIO Pad Data
Output/Input (PGPDO/PGPDI) and they are 16bit wide. I will try to
find a better name for this.
>> +/**
>> + * struct siul2_device_data - platform data attached to the compatible.
>> + * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
>> + * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
>> + */
>> +struct siul2_device_data {
>> + const struct regmap_access_table **pad_access;
>> + const bool reset_cnt;
>
> I don't understand the reset_cnt variable at all. Explain why it exists in the
> kerneldoc please.
It is used while generating the GPIO line names. The naming scheme is to
have: P + letter + number(0 to 15). The "reset_cnt" is used to tell if
the number should change to 0 when naming the SIUL21 GPIOs. For example,
for S32G2, the SIUL20 module has 102 pins named PA00, PA01, ..., PA15,
PB0, .., PG05. SIUL21 starts at 112 and reset_cnt is true so the first
pin will be PH00.
We have another platform which uses this driver and doesn't have the 102-111 gap
and the first SIUL21 pin is named PH06.
I will remove this. I figured out now that I can see if the first pin is
divisible by 16 and reset the counter in that case.
>
>> +/**
>> + * struct siul2_desc - describes a SIUL2 hw module.
>> + * @pad_access: array of valid I/O pads.
>
> Now that is really pin control isn't it.
This is again referring to the registers for setting the
the output value/reading the input value of a GPIO. I will
change the name in v5.
>
>> + * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
>> + * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
>> + * @gpio_base: the first GPIO pin.
>> + * @gpio_num: the number of GPIO pins.
>> + */
>> +struct siul2_desc {
>> + const struct regmap_access_table *pad_access;
>> + struct regmap *opadmap;
>> + struct regmap *ipadmap;
>> + u32 gpio_base;
>> + u32 gpio_num;
>> +};
> (...)
>
>> +static int siul2_get_gpio_pinspec(struct platform_device *pdev,
>> + struct of_phandle_args *pinspec,
>> + unsigned int range_index)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> +
>> + return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
>> + range_index, pinspec);
>> +}
>
> I do not see why a driver would parse gpio-ranges since the gpiolib
> core should normally deal with the translation between GPIO lines
> and pins.
>
> This looks hacky and probably an indication that you want to use
> a combined pinctrl+gpio driver so the different parts of the driver
> can access the same structures easily without hacks.
We parse the gpio-ranges property because this driver handles
two GPIO hardware IPs in a single driver instance. This is because
both SIUL2 modules have pins with interrupt capability but the
registers to configure interrupts are part of SIUL21.
We use the gpio_base and gpio_num to generate the line names
and to select the correct regmap for PGPDOs/PGPDIs
(Parallel GPIO Pad Data Output/Input).
>
>> +static void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
>> + unsigned int gpio, int dir)
>> +{
>> + guard(raw_spinlock_irqsave)(&dev->lock);
>> +
>> + if (dir == GPIO_LINE_DIRECTION_IN)
>> + __clear_bit(gpio, dev->pin_dir_bitmap);
>> + else
>> + __set_bit(gpio, dev->pin_dir_bitmap);
>> +}
>
> This looks like a job for gpio regmap?
>
> select GPIO_REGMAP,
> look in other driver for examples of how to use it.
We use the bitmap just to store the current direction of a pad.
I don't think we can use the gpio-regmap driver because we
rely on the pinctrl driver to configure the pin(pinmux&pinconf).
>
>> +static int siul2_get_direction(struct siul2_gpio_dev *dev,
>> + unsigned int gpio)
>> +{
>> + return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
>> + GPIO_LINE_DIRECTION_IN;
>> +}
>
> Also looks like a reimplementation of GPIO_REGMAP.
The answer above applies here as well.
>
>> +static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
>> + int val)
>> +{
>> + struct siul2_gpio_dev *gpio_dev;
>> + int ret = 0;
>> +
>> + gpio_dev = to_siul2_gpio_dev(chip);
>> + siul2_gpio_set_val(chip, gpio, val);
>> +
>> + ret = pinctrl_gpio_direction_output(chip, gpio);
> (...)
>
> This is nice, pin control is properly used as the back-end.
>
>> +static int siul2_gpio_remove_reserved_names(struct device *dev,
>> + struct siul2_gpio_dev *gpio_dev,
>> + char **names)
>> +{
>> + struct device_node *np = dev->of_node;
>> + int num_ranges, i, j, ret;
>> + u32 base_gpio, num_gpio;
>> +
>> + /* Parse the gpio-reserved-ranges to know which GPIOs to exclude. */
>> +
>> + num_ranges = of_property_count_u32_elems(dev->of_node,
>> + "gpio-reserved-ranges");
>> +
>> + /* The "gpio-reserved-ranges" is optional. */
>> + if (num_ranges < 0)
>> + return 0;
>> + num_ranges /= 2;
>> +
>> + for (i = 0; i < num_ranges; i++) {
>> + ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
>> + i * 2, &base_gpio);
>> + if (ret) {
>> + dev_err(dev, "Could not parse the start GPIO: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
>> + i * 2 + 1, &num_gpio);
>> + if (ret) {
>> + dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
>> + dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Remove names set for reserved GPIOs. */
>> + for (j = base_gpio; j < base_gpio + num_gpio; j++) {
>> + devm_kfree(dev, names[j]);
>> + names[j] = NULL;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>
> I don't get this. gpio-reserved-ranges is something that is parsed and
> handled by gpiolib. Read the code in gpiolib.c and you'll probably
> figure out how to let gpiolib take care of this for you.
We use this just to remove line names for reserved GPIOs(in our case
not present, we have some gaps).
>
>> +static int siul2_gpio_populate_names(struct device *dev,
>> + struct siul2_gpio_dev *gpio_dev)
>> +{
>> + unsigned int num_index = 0;
>> + char ch_index = 'A';
>> + char **names;
>> + int i, ret;
>> +
>> + names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
>> + GFP_KERNEL);
>> + if (!names)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < S32G2_SIUL2_NUM; i++) {
>> + ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
>> + names + gpio_dev->siul2[i].gpio_base,
>> + &ch_index, &num_index);
>> + if (ret) {
>> + dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
>> + i);
>> + return ret;
>> + }
>> +
>> + if (gpio_dev->platdata->reset_cnt)
>> + num_index = 0;
>> +
>> + ch_index++;
>> + }
>> +
>> + ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
>> + if (ret)
>> + return ret;
>> +
>> + gpio_dev->gc.names = (const char *const *)names;
>> +
>> + return 0;
>> +}
>
> Interesting!
>
> I'm not against, on the contrary this looks really helpful to users.
I can try to integrate this into gpiolib after I merge this driver.
>
>> + gc = &gpio_dev->gc;
>
> No poking around in the gpiolib internals please.
>
> Look at what other drivers do and do like they do.
I will fix in v5.
>
> On top of these comments:
> everywhere you do [raw_spin_]locks: try to use guards or
> scoped guards instead. git grep guard drivers/gpio for
> many examples.
I will fix in v5.
>
> Yours,
> Linus Walleij
Best regards,
Andrei
Hi Andrei,
kernel test robot noticed the following build errors:
[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.11 next-20240927]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrei-Stefanescu/drivers-provide-devm_platform_get_and_ioremap_resource_byname/20240926-223448
base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link: https://lore.kernel.org/r/20240926143122.1385658-4-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240928/202409281117.IOrJBXgL-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 7773243d9916f98ba0ffce0c3a960e4aa9f03e81)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240928/202409281117.IOrJBXgL-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409281117.IOrJBXgL-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> drivers/gpio/gpio-siul2-s32g2.c:296:32: warning: comparison of distinct pointer types ('typeof ((size)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
296 | conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
>> drivers/gpio/gpio-siul2-s32g2.c:296:32: error: incompatible pointer types passing 'resource_size_t *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
296 | conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ^
>> drivers/gpio/gpio-siul2-s32g2.c:296:32: warning: shift count >= width of type [-Wshift-count-overflow]
296 | conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^ ~~
include/linux/compiler.h:76:40: note: expanded from macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
8 warnings and 1 error generated.
vim +296 drivers/gpio/gpio-siul2-s32g2.c
273
274 static struct regmap *common_regmap_init(struct platform_device *pdev,
275 struct regmap_config *conf,
276 const char *name)
277 {
278 struct device *dev = &pdev->dev;
279 struct resource *res;
280 resource_size_t size;
281 void __iomem *base;
282
283 base = devm_platform_get_and_ioremap_resource_byname(pdev, name, &res);
284 if (IS_ERR(base)) {
285 dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
286 return ERR_PTR(-EINVAL);
287 }
288
289 size = resource_size(res);
290 conf->val_bits = conf->reg_stride * 8;
291 conf->max_register = size - conf->reg_stride;
292 conf->name = name;
293 conf->use_raw_spinlock = true;
294
295 if (conf->cache_type != REGCACHE_NONE)
> 296 conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
297
298 return devm_regmap_init_mmio(dev, base, conf);
299 }
300
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Andrei,
kernel test robot noticed the following build warnings:
[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.11 next-20240927]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrei-Stefanescu/drivers-provide-devm_platform_get_and_ioremap_resource_byname/20240926-223448
base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link: https://lore.kernel.org/r/20240926143122.1385658-4-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
config: mips-randconfig-r123-20240928 (https://download.01.org/0day-ci/archive/20240928/202409280936.0GoVcXMc-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20240928/202409280936.0GoVcXMc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409280936.0GoVcXMc-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/gpio/gpio-siul2-s32g2.c:296:46: sparse: sparse: incompatible types in comparison expression (different type sizes):
drivers/gpio/gpio-siul2-s32g2.c:296:46: sparse: unsigned int *
drivers/gpio/gpio-siul2-s32g2.c:296:46: sparse: unsigned long long [usertype] *
drivers/gpio/gpio-siul2-s32g2.c:143:9: sparse: sparse: context imbalance in 'siul2_gpio_set_direction' - wrong count at exit
drivers/gpio/gpio-siul2-s32g2.c:296:46: sparse: sparse: shift too big (32) for type unsigned int
vim +296 drivers/gpio/gpio-siul2-s32g2.c
273
274 static struct regmap *common_regmap_init(struct platform_device *pdev,
275 struct regmap_config *conf,
276 const char *name)
277 {
278 struct device *dev = &pdev->dev;
279 struct resource *res;
280 resource_size_t size;
281 void __iomem *base;
282
283 base = devm_platform_get_and_ioremap_resource_byname(pdev, name, &res);
284 if (IS_ERR(base)) {
285 dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
286 return ERR_PTR(-EINVAL);
287 }
288
289 size = resource_size(res);
290 conf->val_bits = conf->reg_stride * 8;
291 conf->max_register = size - conf->reg_stride;
292 conf->name = name;
293 conf->use_raw_spinlock = true;
294
295 if (conf->cache_type != REGCACHE_NONE)
> 296 conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
297
298 return devm_regmap_init_mmio(dev, base, conf);
299 }
300
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Andrei,
kernel test robot noticed the following build errors:
[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.11 next-20240926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrei-Stefanescu/drivers-provide-devm_platform_get_and_ioremap_resource_byname/20240926-223448
base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link: https://lore.kernel.org/r/20240926143122.1385658-4-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20240927/202409271406.SOjGw98h-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240927/202409271406.SOjGw98h-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409271406.SOjGw98h-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from ./arch/openrisc/include/generated/asm/div64.h:1,
from include/linux/math.h:6,
from include/linux/kernel.h:27,
from include/linux/cpumask.h:11,
from include/linux/smp.h:13,
from include/linux/lockdep.h:14,
from include/linux/spinlock.h:63,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/mm.h:7,
from arch/openrisc/include/asm/pgalloc.h:20,
from arch/openrisc/include/asm/io.h:18,
from include/linux/io.h:14,
from drivers/gpio/gpio-siul2-s32g2.c:11:
drivers/gpio/gpio-siul2-s32g2.c: In function 'common_regmap_init':
include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast [-Wcompare-distinct-pointer-types]
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ^~
drivers/gpio/gpio-siul2-s32g2.c:296:46: note: in expansion of macro 'do_div'
296 | conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
| ^~~~~~
In file included from include/linux/err.h:5,
from drivers/gpio/gpio-siul2-s32g2.c:9:
include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^~
include/linux/compiler.h:76:45: note: in definition of macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/gpio/gpio-siul2-s32g2.c:296:46: note: in expansion of macro 'do_div'
296 | conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
| ^~~~~~
>> include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Wincompatible-pointer-types]
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
| |
| resource_size_t * {aka unsigned int *}
drivers/gpio/gpio-siul2-s32g2.c:296:46: note: in expansion of macro 'do_div'
296 | conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
| ^~~~~~
include/asm-generic/div64.h:213:38: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'resource_size_t *' {aka 'unsigned int *'}
213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
| ~~~~~~~~~~^~~~~~~~
vim +/__div64_32 +238 include/asm-generic/div64.h
^1da177e4c3f41 Linus Torvalds 2005-04-16 215
^1da177e4c3f41 Linus Torvalds 2005-04-16 216 /* The unnecessary pointer compare is there
^1da177e4c3f41 Linus Torvalds 2005-04-16 217 * to check for type safety (n must be 64bit)
^1da177e4c3f41 Linus Torvalds 2005-04-16 218 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 219 # define do_div(n,base) ({ \
^1da177e4c3f41 Linus Torvalds 2005-04-16 220 uint32_t __base = (base); \
^1da177e4c3f41 Linus Torvalds 2005-04-16 221 uint32_t __rem; \
^1da177e4c3f41 Linus Torvalds 2005-04-16 222 (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
911918aa7ef6f8 Nicolas Pitre 2015-11-02 223 if (__builtin_constant_p(__base) && \
911918aa7ef6f8 Nicolas Pitre 2015-11-02 224 is_power_of_2(__base)) { \
911918aa7ef6f8 Nicolas Pitre 2015-11-02 225 __rem = (n) & (__base - 1); \
911918aa7ef6f8 Nicolas Pitre 2015-11-02 226 (n) >>= ilog2(__base); \
c747ce4706190e Geert Uytterhoeven 2021-08-11 227 } else if (__builtin_constant_p(__base) && \
461a5e51060c93 Nicolas Pitre 2015-10-30 228 __base != 0) { \
461a5e51060c93 Nicolas Pitre 2015-10-30 229 uint32_t __res_lo, __n_lo = (n); \
461a5e51060c93 Nicolas Pitre 2015-10-30 230 (n) = __div64_const32(n, __base); \
461a5e51060c93 Nicolas Pitre 2015-10-30 231 /* the remainder can be computed with 32-bit regs */ \
461a5e51060c93 Nicolas Pitre 2015-10-30 232 __res_lo = (n); \
461a5e51060c93 Nicolas Pitre 2015-10-30 233 __rem = __n_lo - __res_lo * __base; \
911918aa7ef6f8 Nicolas Pitre 2015-11-02 234 } else if (likely(((n) >> 32) == 0)) { \
^1da177e4c3f41 Linus Torvalds 2005-04-16 235 __rem = (uint32_t)(n) % __base; \
^1da177e4c3f41 Linus Torvalds 2005-04-16 236 (n) = (uint32_t)(n) / __base; \
c747ce4706190e Geert Uytterhoeven 2021-08-11 237 } else { \
^1da177e4c3f41 Linus Torvalds 2005-04-16 @238 __rem = __div64_32(&(n), __base); \
c747ce4706190e Geert Uytterhoeven 2021-08-11 239 } \
^1da177e4c3f41 Linus Torvalds 2005-04-16 240 __rem; \
^1da177e4c3f41 Linus Torvalds 2005-04-16 241 })
^1da177e4c3f41 Linus Torvalds 2005-04-16 242
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2024 Red Hat, Inc.