Message-ID: <20250223030917-GYA33864@gentoo>
Date: Sun, 23 Feb 2025 03:09:17 +0000
From: Yixun Lan <dlan@...too.org>
To: Alex Elder <elder@...e.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Conor Dooley <conor@...nel.org>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>, Yangyu Chen <cyy@...self.name>,
	Jisheng Zhang <jszhang@...nel.org>,
	Jesse Taube <mr.bossman075@...il.com>,
	Inochi Amaoto <inochiama@...look.com>,
	Icenowy Zheng <uwu@...nowy.me>,
	Meng Zhang <zhangmeng.kevin@...ux.spacemit.com>,
	linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
	spacemit@...ts.linux.dev
Subject: Re: [PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

Hi Alex:

thanks for the review

On 11:27 Fri 21 Feb     , Alex Elder wrote:
> On 2/17/25 6:57 AM, Yixun Lan wrote:
> > Implement GPIO functionality which capable of setting pin as
> > input, output. Also, each pin can be used as interrupt which
> > support rising, failing, or both edge type trigger.
> > 
> > Signed-off-by: Yixun Lan <dlan@...too.org>
> 
> This looks nicer!
> 
> I have some more comments, but they're pretty minor.
> 
> > ---
> >   drivers/gpio/Kconfig            |   8 +
> >   drivers/gpio/Makefile           |   1 +
> >   drivers/gpio/gpio-spacemit-k1.c | 376 ++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 385 insertions(+)
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index add5ad29a673c09082a913cb2404073b2034af48..eaae729eec00a3d6d2b83769aed3e2b0ca9927e5 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -655,6 +655,14 @@ config GPIO_SNPS_CREG
> >   	  where only several fields in register belong to GPIO lines and
> >   	  each GPIO line owns a field with different length and on/off value.
> >   
> > +config GPIO_SPACEMIT_K1
> > +	bool "SPACEMIT K1 GPIO support"
> > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > +	depends on OF_GPIO
> > +	select GPIOLIB_IRQCHIP
> > +	help
> > +	  Say yes here to support the SpacemiT's K1 GPIO device.
> > +
> >   config GPIO_SPEAR_SPICS
> >   	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
> >   	depends on PLAT_SPEAR
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index af3ba4d81b583842893ea69e677fbe2abf31bc7b..6709ce511a0cf10310a94521c85a2d382dcfa696 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -156,6 +156,7 @@ obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.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
> > +obj-$(CONFIG_GPIO_SPACEMIT_K1)		+= gpio-spacemit-k1.o
> >   obj-$(CONFIG_GPIO_SPEAR_SPICS)		+= gpio-spear-spics.o
> >   obj-$(CONFIG_GPIO_SPRD)			+= gpio-sprd.o
> >   obj-$(CONFIG_GPIO_STMPE)		+= gpio-stmpe.o
> > diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..f72511b5ab8f8f0b1d1c9e89d2f9ca07b623a866
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-spacemit-k1.c
> > @@ -0,0 +1,376 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/*
> > + * Copyright (C) 2023-2025 SpacemiT (Hangzhou) Technology Co. Ltd
> > + * Copyright (C) 2025 Yixun Lan <dlan@...too.org>
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/init.h>
> > +#include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/property.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/module.h>
> > +
> > +#include "gpiolib.h"
> > +
> > +/* register offset */
> 
> The comments are great, but I think I'd like to see them be abbreviated
> further and added to the right of the definitions, if you can do that.
> 
> I think you can drop "GPIO" and "register" in each one of them, and
> that might get you close to an 80 column limit.  See what you can do.
> 
sure, I can do this
> > +/* GPIO port level register */
> 
> I think the port level register is read-only, and you didn't include
> that annotation.
> 
right, I will add that annotation
> > +#define GPLR		0x00
> > +/* GPIO port direction register - R/W */
> > +#define GPDR		0x0c
> > +/* GPIO port set register - W */
> > +#define GPSR		0x18
> > +/* GPIO port clear register - W */
> > +#define GPCR		0x24
> > +/* GPIO port rising edge register R/W */
> > +#define GRER		0x30
> > +/* GPIO port falling edge register R/W */
> > +#define GFER		0x3c
> > +/* GPIO edge detect status register - R/W1C */
> > +#define GEDR		0x48
> > +/*  GPIO (set) direction register - W */
> 
> Delete the extra space above.
> 
will do
> > +#define GSDR		0x54
> > +/* GPIO (clear) direction register - W */
> > +#define GCDR		0x60
> > +/* GPIO (set) rising edge detect enable register - W */
> > +#define GSRER		0x6c
> > +/* GPIO (clear) rising edge detect enable register - W */
> > +#define GCRER		0x78
> > +/* GPIO (set) falling edge detect enable register - W */
> > +#define GSFER		0x84
> > +/* GPIO (clear) falling edge detect enable register - W */
> > +#define GCFER		0x90
> > +/* GPIO interrupt mask register, 0 disable, 1 enable - R/W */
> > +#define GAPMASK		0x9c
> > +
> > +#define NR_BANKS		4
> > +#define NR_GPIOS_PER_BANK	32
> > +
> > +#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> > +
> > +struct spacemit_gpio;
> > +
> > +struct spacemit_gpio_bank {
> > +	struct gpio_chip		gc;
> > +	struct spacemit_gpio		*sg;
> > +	void __iomem			*base;
> > +	u32				index;
> 
> You almost never use the index field.  It could easily be
> computed rather than stored:
> 
> static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
> {
> 	return (u32)(gb - gb->sg->sgb);
> }
> 
ok
> > +	u32				irq_mask;
> > +	u32				irq_rising_edge;
> > +	u32				irq_falling_edge;
> > +};
> > +
> > +struct spacemit_gpio {
> > +	struct	device			*dev;
> > +	struct	spacemit_gpio_bank	sgb[NR_BANKS];
> > +};
> > +
> > +static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> > +{
> > +	struct spacemit_gpio_bank *gb = dev_id;
> > +	unsigned long pending;
> > +	u32 n, gedr;
> > +
> > +	gedr = readl(gb->base + GEDR);
> > +	if (!gedr)
> > +		return IRQ_NONE;
> > +	writel(gedr, gb->base + GEDR);
> > +
> > +	gedr = gedr & gb->irq_mask;
> > +	if (!gedr)
> > +		return IRQ_NONE;
> > +
> > +	pending = gedr;
> 
> Instead, do:
> 
> 	pending = gedr & gb->irq_mask;
> 	if (!pending)
> 		return IRQ_NONE;
> 
good suggestion
> > +	for_each_set_bit(n, &pending, BITS_PER_LONG)
> > +		handle_nested_irq(irq_find_mapping(gb->gc.irq.domain, n));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void spacemit_gpio_irq_ack(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> > +
> > +	writel(BIT(irqd_to_hwirq(d)), gb->base + GEDR);
> > +}
> > +
> > +static void spacemit_gpio_irq_mask(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> > +	gb->irq_mask &= ~bit;
> 
> This is a minor suggestion, and I'm not sure how much difference
> it makes.  But here (and one or two more times below) you could
> avoid the writel() calls if you know the particular IRQ was
> already disabled.  (Maybe that won't ever happen?)
> 
pratically, this should be called once irq unmasked (in pair),
besides, it won't worth to do the optimization as not called frequently

> 	if (!(gb->irq_mask & bit))
> 		return;
> 
> 	gb->irq_mask &= !bit;
> 	...
> 
> This should work because in spacemit_gpio_add_bank() you reset
> all the IRQ state and disable all IRQs, so the cached copy of
> the irq_mask and the rising and falling edge masks should match
> reality.
> 
> > +
> > +	if (bit & gb->irq_rising_edge)
> > +		writel(bit, gb->base + GCRER);
> > +
> > +	if (bit & gb->irq_falling_edge)
> > +		writel(bit, gb->base + GCFER);
> > +}
> > +
> > +static void spacemit_gpio_irq_unmask(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> 
> 
> Same thought here.
> 
> 	if (gb->irq_mask & bit)
> 		return;
> 
> > +	gb->irq_mask |= bit;
> > +
> > +	if (bit & gb->irq_rising_edge)
> > +		writel(bit,  gb->base + GSRER);
> > +
> > +	if (bit & gb->irq_falling_edge)
> > +		writel(bit, gb->base + GSFER);
> > +}
> > +
> > +static int spacemit_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> 
> Same thought in this function, although it gets a little
> messier looking.
> 
> > +	if (type & IRQ_TYPE_EDGE_RISING) {
> > +		gb->irq_rising_edge |= bit;
> > +		writel(bit, gb->base + GSRER);
> > +	} else {
> > +		gb->irq_rising_edge &= ~bit;
> > +		writel(bit, gb->base + GCRER);
> > +	}
> 
> Otherwise:
> 
>      if (type & IRQ_TYPE_EDGE_RISING)
> 	gb->irq_rising_edge |= bit;
>      else
> 	gb->irq_rising_edge &= ~bit;
>      writel(bit, gb->base + GSRER);
> 
no, it's two different registers: GSRER vs GCRER
> and again below.
> 
> > +
> > +	if (type & IRQ_TYPE_EDGE_FALLING) {
> > +		gb->irq_falling_edge |= bit;
> > +		writel(bit, gb->base + GSFER);
> > +	} else {
> > +		gb->irq_falling_edge &= ~bit;
> > +		writel(bit, gb->base + GCFER);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> You added this function in version 5 of the series.  Please call
> attention to additions (or removals) like this in your cover page,
> and/or in notes at the top of this patch.
> 
ok
> > +static void spacemit_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(data);
> > +
> > +	seq_printf(p, "%s-%d", dev_name(gb->gc.parent), gb->index);
> 
> Does this look like "gpiochip2-15" or something?  I wasn't able
> to find it in the debugfs file system.
> 
it shows in /proc/interrupts once irq registered..
something will look like this - d4019000.gpio-$index

> > +}
> > +
> > +static struct irq_chip spacemit_gpio_chip = {
> > +	.name		= "k1-gpio-irqchip",
> > +	.irq_ack	= spacemit_gpio_irq_ack,
> > +	.irq_mask	= spacemit_gpio_irq_mask,
> > +	.irq_unmask	= spacemit_gpio_irq_unmask,
> > +	.irq_set_type	= spacemit_gpio_irq_set_type,
> > +	.irq_print_chip	= spacemit_gpio_irq_print_chip,
> > +	.flags		= IRQCHIP_IMMUTABLE,
> 
> Last time your flags value was IRQCHIP_SET_WAKE.  Why the change?
> 
I was about to check this..

the gpio controller doesn't support irq wake up,
I will add this flag in next version

> > +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> > +};
> > +
> 
> Maybe you could add a comment indicating that gpiospec->args[]
> will contain:
> 0:  bank index
> 1:  GPIO offset within the bank
> 2:  flags
> 
> (And the GPIO chip instance number as Linus suggested.)
> 
please ignore, I will drop this function as LinusW promote this
to gpio core
> > +static int spacemit_gpio_xlate(struct gpio_chip *gc,
> > +			       const struct of_phandle_args *gpiospec, u32 *flags)
> > +{
> > +	struct spacemit_gpio_bank *gb = gpiochip_get_data(gc);
> > +	struct spacemit_gpio *sg = gb->sg;
> > +
> 
> Get rid of the above blank line.
> 
> > +	int i;
> > +
> 
> I'm not sure the context in which this runs.  Can it be given
> arbitrary content from a DTB?  Mainly I'm interested to know
> whether any of these checks can be eliminated.  If it's called
> while parsing a DTB I can see why you'd need to verify all
> input values for validity.
> 
> > +	if (gc->of_gpio_n_cells != 3)
> > +		return -EINVAL;
> > +
> > +	if (gpiospec->args_count < gc->of_gpio_n_cells)
> > +		return -EINVAL;
> > +
> > +	i = gpiospec->args[0];
> > +	if (i >= NR_BANKS)
> > +		return -EINVAL;
> > +
> > +	if (gc != &sg->sgb[i].gc)
> > +		return -EINVAL;
> > +
> > +	if (gpiospec->args[1] >= gc->ngpio)
> > +		return -EINVAL;
> > +
> > +	if (flags)
> > +		*flags = gpiospec->args[2];
> > +
> > +	return gpiospec->args[1];
> > +}
> > +
> > +static int spacemit_add_pin_range(struct gpio_chip *gc)
> > +{
> > +	struct spacemit_gpio_bank *gb;
> > +	struct of_phandle_args pinspec;
> > +	struct pinctrl_dev *pctldev;
> > +	struct device_node *np;
> > +	int ret, trim;
> > +
> > +	np = dev_of_node(&gc->gpiodev->dev);
> > +	if (!np)
> > +		return 0;
> > +
> > +	gb = to_spacemit_gpio_bank(gc);
> > +
> > +	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> > +					       gb->index, &pinspec);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pctldev = of_pinctrl_get(pinspec.np);
> > +	of_node_put(pinspec.np);
> > +	if (!pctldev)
> > +		return -EPROBE_DEFER;
> > +
> > +	/* Ignore ranges outside of this GPIO chip */
> > +	if (pinspec.args[0] >= (gc->offset + gc->ngpio))
> > +		return -EINVAL;
> > +
> > +	if (pinspec.args[0] + pinspec.args[2] <= gc->offset)
> > +		return -EINVAL;
> > +
> 
> I would do the following test earlier.
> 
ditto, ignore this, as move to gpio core
> > +	if (!pinspec.args[2])
> > +		return -EINVAL;
> > +
> > +	/* Trim the range to fit this GPIO chip */
> > +	if (gc->offset > pinspec.args[0]) {
> > +		trim = gc->offset - pinspec.args[0];
> > +		pinspec.args[2] -= trim;
> > +		pinspec.args[1] += trim;
> > +		pinspec.args[0] = 0;
> > +	} else {
> > +		pinspec.args[0] -= gc->offset;
> > +	}
> > +	if ((pinspec.args[0] + pinspec.args[2]) > gc->ngpio)
> > +		pinspec.args[2] = gc->ngpio - pinspec.args[0];
> > +
> > +	ret = gpiochip_add_pin_range(gc,
> > +				     pinctrl_dev_get_devname(pctldev),
> > +				     pinspec.args[0],
> > +				     pinspec.args[1],
> > +				     pinspec.args[2]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> Just do this:
> 
>      return  gpiochip_add_pin_range(gc, pinctrl_dev_get_devname(pctldev),
> 				   pinspec.args[0], pinspec.args[2]);
> 
> > +}
> > +
> > +static int spacemit_gpio_add_bank(struct spacemit_gpio *sg,
> > +				  void __iomem *regs,
> > +				  int index, int irq)
> > +{
> > +	struct spacemit_gpio_bank *gb = &sg->sgb[index];
> > +	struct gpio_chip *gc = &gb->gc;
> > +	struct device *dev = sg->dev;
> > +	struct gpio_irq_chip *girq;
> > +	void __iomem *dat, *set, *clr, *dirin, *dirout;
> > +	int ret, bank_base[] = { 0x0, 0x4, 0x8, 0x100 };
> > +
> > +	gb->index = index;
> > +	gb->base = regs + bank_base[index];
> > +
> > +	dat	= gb->base + GPLR;
> > +	set	= gb->base + GPSR;
> > +	clr	= gb->base + GPCR;
> > +	dirin	= gb->base + GCDR;
> > +	dirout	= gb->base + GSDR;
> > +
> > +	/* This registers 32 GPIO lines per bank */
> > +	ret = bgpio_init(gc, dev, 4, dat, set, clr, dirout, dirin,
> > +			 BGPIOF_UNREADABLE_REG_SET | BGPIOF_UNREADABLE_REG_DIR);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to init gpio chip\n");
> > +
> > +	gb->sg = sg;
> > +
> > +	gc->label		= dev_name(dev);
> > +	gc->request		= gpiochip_generic_request;
> > +	gc->free		= gpiochip_generic_free;
> > +	gc->ngpio		= NR_GPIOS_PER_BANK;
> > +	gc->base		= -1;
> > +
> > +#ifdef CONFIG_OF_GPIO
> 
> Why are these lines conditionally defined?  Is it intended
> to allow CONFIG_COMPILE_TEST to work?  Your Kconfig states
> that this *depends on* OF_GPIO, so this is probably not
> needed.
> 
> You don't define spacemit_gpio_xlate() earlier conditionally.
> Nor spacemit_add_pin_range().
> 
make sense, I will drop this #ifdef
> > +	gc->of_xlate		= spacemit_gpio_xlate;
> > +	gc->of_add_pin_range	= spacemit_add_pin_range;
> > +	gc->of_gpio_n_cells	= 3;
> > +#endif
> > +
> > +	girq			= &gc->irq;
> > +	girq->threaded		= true;
> > +	girq->handler		= handle_simple_irq;
> > +
> > +	gpio_irq_chip_set_chip(girq, &spacemit_gpio_chip);
> > +
> > +	/* Clear Edge Detection Settings */
> > +	writel(0x0, gb->base + GRER);
> > +	writel(0x0, gb->base + GFER);
> > +	/* Clear and Disable Interrupt */
> > +	writel(0xffffffff, gb->base + GCFER);
> > +	writel(0xffffffff, gb->base + GCRER);
> 
> It seems that GAPMASK is an overall interrupt mask register.
> I assume that means that by writing 0 here, no interrupts
> of any kind will be generated for any of the 32 GPIO ports.
> 
yes

> If that's true, I would write this first, *then* disable
> the rising and falling edge detection interrupts, *then*
> clear any pending interrupts.
> 
ok, I will take your suggestion, this is more strict

> Are there any interrupt types other than rising and falling
> edge?  Does this just provide an atomic way to disable both
only two types, rising, falling, and both can be enabled simultaneously
(there is no level trigger interrupt)

> types at once?  If there are no other interrupt types maybe
> this could be used rather than disabling both types
> separately using GCFER etc. in spacemit_gpio_irq_*mask().
> 
you are right, I think we can do this

> 					-Alex
> 
> > +	writel(0, gb->base + GAPMASK);
> > +
> > +	ret = devm_request_threaded_irq(dev, irq, NULL,
> > +					spacemit_gpio_irq_handler,
> > +					IRQF_ONESHOT | IRQF_SHARED,
> > +					gb->gc.label, gb);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > +	ret = devm_gpiochip_add_data(dev, gc, gb);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to add gpio chip\n");
> > +
> > +	/* Eable Interrupt */
> > +	writel(0xffffffff, gb->base + GAPMASK);
> > +
> > +	return 0;
> > +}
> > +
> > +static int spacemit_gpio_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct spacemit_gpio *sg;
> > +	struct resource *res;
> > +	void __iomem *regs;
> > +	int i, irq, ret;
> > +
> > +	sg = devm_kzalloc(dev, sizeof(*sg), GFP_KERNEL);
> > +	if (!sg)
> > +		return -ENOMEM;
> > +
> > +	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	sg->dev	= dev;
> > +
> > +	for (i = 0; i < NR_BANKS; i++) {
> > +		ret = spacemit_gpio_add_bank(sg, regs, i, irq);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id spacemit_gpio_dt_ids[] = {
> > +	{ .compatible = "spacemit,k1-gpio" },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver spacemit_gpio_driver = {
> > +	.probe		= spacemit_gpio_probe,
> > +	.driver		= {
> > +		.name	= "k1-gpio",
> > +		.of_match_table = spacemit_gpio_dt_ids,
> > +	},
> > +};
> > +module_platform_driver(spacemit_gpio_driver);
> > +
> > +MODULE_AUTHOR("Yixun Lan <dlan@...too.org>");
> > +MODULE_DESCRIPTION("GPIO driver for SpacemiT K1 SoC");
> > +MODULE_LICENSE("GPL");
> > 
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Powered by blists - more mailing lists