Handle GPIO direction reg offset in driver #90013

Ayush1325 · 2025-05-15T12:04:08Z

Currently, the start address in devicetree for GPIO controllers using gpio-davinci points to the direction register. This is different from Linux dt which points to the start of GPIO controller, and handles getting the address for the direction register in the driver.

Essentially, this means that Zephyr address = Linux address + 0x10.

This can be confusing for anyone not familiar with this quirk. Additionally, the dt value in reg property seems to be invalid since the the size of the region is now ACTUAL SIZE - 0x10.

The Linux kernel driver handles calculating the direction register address for a GPIO bank, so Zephyr should do the same.

I am also introducing the offsets for the other banks in this PR but they cannot be used yet, since Zephyr GPIO subsystem is currently limited to only 32 pins per port (PocketBeagle 2 MAIN_GPIO0 has 92 pins).

dnltz · 2025-05-15T12:12:51Z

drivers/gpio/gpio_davinci.c

+
+static struct gpio_davinci_regs *gpio_davinci_get_regs(const struct device *dev, uint8_t bank)
+{
+	if (bank < MAX_REGS_BANK) {


Add an assert or fail here instead of returning NULL and fail somewhere else.

drivers/gpio/gpio_davinci.c

dnltz · 2025-05-15T12:18:20Z

drivers/gpio/gpio_davinci.c

+const unsigned int offset_array[5] = {0x10, 0x38, 0x60, 0x88, 0xb0};
+#define MAX_REGS_BANK ARRAY_SIZE(offset_array)
+
+static struct gpio_davinci_regs *gpio_davinci_get_regs(const struct device *dev, uint8_t bank)


What's your plan on how to calculate this bank value?

So, the bank value is basically pin / 32.

static uint32_t __gpio_mask(gpio_pin_t gpio) { return BIT(gpio % 32); } static uint8_t __gpio_bank(gpio_pin_t gpio) { return gpio / 32; }

It technically does not seem to call it bank since well, the Ti docs defines bank differently from Linux kernel driver. But I am following the Linux driver definition here.

I think passing the pin instead of bank and calculating the bank number would make that function more suitable for future support for multiple banks.

Passing a single pin should work fine, but then it cannot be used to set multiple pins at once for a bank like it is done right now for BANK0

ashmrvl · 2025-05-16T05:24:56Z

drivers/gpio/gpio_davinci.c

@@ -64,10 +61,20 @@ struct gpio_davinci_config {
 	const struct pinctrl_dev_config *pcfg;
 };

+const unsigned int offset_array[5] = {0x10, 0x38, 0x60, 0x88, 0xb0};


Can you give some other name instead of offset_array? like dir_reg_start_addr[].
Can you create macro for array size? #define NO_OF_BANKS 5

Can you give some other name instead of offset_array? like dir_reg_start_addr[].

I can, but do note that Linux kernel calls it offset_array, so I don't see much reason to change it.

Can you create macro for array size? #define NO_OF_BANKS 5

It is already defined. See the line below it:

#define MAX_REGS_BANK ARRAY_SIZE(offset_array)

ashmrvl · 2025-05-16T05:29:44Z

drivers/gpio/gpio_davinci.c

 static int gpio_davinci_configure(const struct device *dev, gpio_pin_t pin,
 					gpio_flags_t flags)
 {
-	volatile struct gpio_davinci_regs *regs = DEV_GPIO_CFG_BASE(dev);
+	volatile struct gpio_davinci_regs *regs = gpio_davinci_get_regs(dev, 0);


Are you planning to implement the __gpio_bank(gpio_pin_t gpio) as part of this PR? If yes, Can you pass pin number to gpio_davinci_get_regs(dev,pin)?

Are you planning to implement the __gpio_bank(gpio_pin_t gpio) as part of this PR?

Nope, the reason is that well, the current driver (and zephyr gpio subsystem) does not support more than 32 pins per port (aka only bank 0 is supported right now).

I will be creating an RFC first to figure out how the GPIO driver API should be changed to support more pins, and then make appropriate changes in this driver.

If yes, Can you pass pin number to gpio_davinci_get_regs(dev,pin)?

Well, whether the bank calculation should be done inside gpio_davinci_get_regs or not depends on the discussion regarding the GPIO driver API. In Linux kernel, the davinci driver only supports actions on individual pins (so setting multiple pin values take multiple writes to the registers). I am assuming, this might not be the popular approach in Zephyr.

Just a general note (which you might be already aware of)

When the gpio driver is changed to consider more than one bank,
for the ti chips am62x_m4, am64x_m4 etc, the last bank registers (from 0x042010B0 to 0x042010D4),
has support for only 16gpio pins and remaining 16bits are reserved.

This needs to be taken care in gpio_davinci.

Good point. We basically need to support ngpios in this driver.

ashmrvl · 2025-05-16T05:48:17Z

drivers/gpio/gpio_davinci.c

@@ -94,7 +101,7 @@ static int gpio_davinci_configure(const struct device *dev, gpio_pin_t pin,
 static int gpio_davinci_port_get_raw(const struct device *dev,
 					gpio_port_value_t *value)
 {
-	volatile struct gpio_davinci_regs *regs = DEV_GPIO_CFG_BASE(dev);
+	volatile struct gpio_davinci_regs *regs = gpio_davinci_get_regs(dev, 0);


For now, will it be okay to create macro BANK0 0 and call gpio_davinci_get_regs(dev, BANK0);?

Currently, the Zephyr dt requires address to the direction register instead of the base GPIO address. usually means base address + 0x10 when compared with Linux. To make things more consistent between linux and zephyr, handle the direction offset in the driver itself. This also lays the foundation for supporting more than 32 GPIOs per port in the future by introducing offsets for all the banks. Signed-off-by: Ayush Singh <ayush@beagleboard.org>

Adjust GPIO davinci memory address Signed-off-by: Ayush Singh <ayush@beagleboard.org>

sonarqubecloud · 2025-05-16T06:19:13Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

natto1784 · 2025-05-21T07:55:27Z

What do you think of adding a driver specific API extensions which one can use to set/get a single pin value like Daniel recommended above

For example

static void gpio_davinci_set_pin(const struct device *dev, uint8_t pin)
{
	volatile struct gpio_davinci_regs *regs;
        uint8_t bank = value / 32;
        uint8_t idx = value % 32;

        regs = gpio_davinci_get_regs(dev, bank);
	regs->set_data |= BIT(idx);
}

static void gpio_davinci_clr_pin(const struct device *dev, uint8_t pin)
{
	volatile struct gpio_davinci_regs *regs;
        uint8_t bank = value / 32;
        uint8_t idx = value % 32;

        regs = gpio_davinci_get_regs(dev, bank);
	regs->set_data &= ~BIT(idx);
}

static bool gpio_davinci_get_pin(const struct device *dev, uint8_t pin)
{
	volatile struct gpio_davinci_regs *regs;
        uint8_t bank = value / 32;
        uint8_t idx = value % 32;

        regs = gpio_davinci_get_regs(dev, bank);
	return !!(regs->set_data & BIT(idx));
}

natto1784 · 2025-05-21T08:01:55Z

drivers/gpio/gpio_davinci.c

+
+#define BANK0 0
+
+static struct gpio_davinci_regs *gpio_davinci_get_regs(const struct device *dev, uint8_t bank)


Can we ALWAYS_INLINE this?

Ayush1325 · 2025-05-21T08:31:34Z

So, I have had some discussions in the past the week, and I am a bit unsure regarding some things.

I think it would be great to move the discussion to the issue: #90057

For starters, I have created a proposal to make the Zephyr GPIO subsystem more like Linux kernel. It will resolve the offsets issue and stuff.

However, according to some old discussions in Zephyr discord, it seems like gpio-nexus nodes are supposed to be used for > 32 lines.

My current understanding seems to be as follows:

/* Bank 1 */
main_gpio0_1 {};

/* Bank 2 */
main_gpio0_2 {};

main_gpio0 {
        compatible = "ti,davinci-gpio-nexus";
        #gpio-cells = <2>;
        gpio-map-mask = <0xffffffff 0xffffffc0>;
        gpio-map-pass-thru = <0 0x3f>;
        gpio-map = <0 0 &main_gpio0_1 0 0>,  
                   <1 0 &main_gpio0_1 1 0>, 
                   ...
                   <32 0 &main_gpio0_2 0 0>, 
                   <33 0 &main_gpio0_2 1 0>; 
                  ...;
};

So, I would like to discuss if we should go with gpio-nexus or changes in the GPIO drivers API.

natto1784 · 2025-05-21T08:54:52Z

I am not a fan of iterating all the pins with the nexus binding...

Ayush1325 · 2025-05-26T05:34:49Z

Hello, if everyone is in agreement that it would be better to have support at subsystem level, can we move ahead this PR so that I can start working on the phase 1 described in #90057

ashmrvl

Since changes for supporting GPIO pins greater than 32 needs changes at subsystem level, approving this PR

github-actions bot added platform: TI SimpleLink area: GPIO platform: TI K3 labels May 15, 2025

github-actions bot requested review from dnltz, glneo, gramsay0, henrikbrixandersen, mnkp, vaishnavachath and vanti May 15, 2025 12:05

github-actions bot assigned vaishnavachath May 15, 2025

Ayush1325 force-pushed the gpio-davinci-linux-address branch from edbc8e3 to b315ef0 Compare May 15, 2025 12:06

dnltz requested changes May 15, 2025

View reviewed changes

Ayush1325 force-pushed the gpio-davinci-linux-address branch 2 times, most recently from 5aa406b to bf0c17b Compare May 15, 2025 17:42

ashmrvl reviewed May 16, 2025

View reviewed changes

Ayush1325 added 2 commits May 16, 2025 11:20

dts: arm: ti: Adjust GPIO memory address

67d00cd

Adjust GPIO davinci memory address Signed-off-by: Ayush Singh <ayush@beagleboard.org>

Ayush1325 force-pushed the gpio-davinci-linux-address branch from bf0c17b to 67d00cd Compare May 16, 2025 05:51

natto1784 reviewed May 21, 2025

View reviewed changes

Ayush1325 requested review from dnltz, natto1784 and ashmrvl May 26, 2025 05:34

ashmrvl approved these changes May 26, 2025

View reviewed changes

natto1784 approved these changes May 26, 2025

View reviewed changes

dnltz approved these changes May 27, 2025

View reviewed changes

vaishnavachath approved these changes May 28, 2025

View reviewed changes

kartben merged commit 42aef1b into zephyrproject-rtos:main May 28, 2025
26 checks passed

Ayush1325 deleted the gpio-davinci-linux-address branch May 28, 2025 06:33


		#define BANK0 0

		static struct gpio_davinci_regs gpio_davinci_get_regs(const struct device dev, uint8_t bank)

Handle GPIO direction reg offset in driver #90013

Handle GPIO direction reg offset in driver #90013

Uh oh!

Conversation

Ayush1325 commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush1325 May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natto1784 May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 16, 2025

Quality Gate passed

Uh oh!

natto1784 commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayush1325 commented May 21, 2025

Uh oh!

natto1784 commented May 21, 2025

Uh oh!

Ayush1325 commented May 26, 2025

Uh oh!

ashmrvl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ayush1325 May 15, 2025 •

edited

Loading

natto1784 May 21, 2025 •

edited

Loading

natto1784 commented May 21, 2025 •

edited

Loading