Re: [PATCH 2/3] pinctrl: ingenic: Add X2600 register and GPIO IRQ support

[H. Nikolaus Schaller]

> Am 13.11.2026 um 10:54 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
> 
> Le vendredi 13 novembre 2026 à 10:49 +0100, H. Nikolaus Schaller a écrit :
>> Hi Paul,
>> 
>>> Am 13.11.2026 um 10:37 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
>>> 
>>> Hi Nikolaus,
>>> 
>>> Le vendredi 13 novembre 2026 à 09:14 +0100, H. Nikolaus Schaller a écrit :
>>>> Add the X2600 SoC ID and adapt the GPIO register access functions
>>>> to its register layout.
>>>> 
>>>> Unlike earlier Ingenic SoCs, the X2600 uses a register bank
>>>> spacing of 0x1000 and provides separate shadow registers and
>>>> load registers for each GPIO port. It also uses a dedicated
>>>> register for clearing GPIO interrupt flags.
>>>> 
>>>> Update the GPIO interrupt configuration to account for hardware
>>>> dual-edge detection on X1600, X2000 and X2600. These SoCs require
>>>> PAT1 to be set when dual-edge detection is enabled, while X1000
>>>> and X1830 continue to use software emulation.
>>> 
>>> So at the same time you're fixing X1600/X2000 and adding support for X2600.
>>> Looks to me like it should be splitted into two patches.
>> 
>> Well, I have to look if it can be split.
>> 
>> So far the driver for X1600/X2000 does not have dual-edge support by
>> hardware (programming the EDG bit).
>> 
>> But registers and programming model are the same. Therefore it is more
>> or less a nice side-effect that x2600 adds functions to x1600/x2000.
>> 
>> I.e. new code would be replacing some is_soc_or_above(jzgc->jzpc, ID_X2600)
>> with is_soc_or_above(jzgc->jzpc, ID_X1600) to fix it for them.
>> 
>> Or otherwise we have to split full x2600 support into two patches.
>> 
>> Is this worth a separate patch or should we leave it as a comment in
>> the commit message?
> 
> My idea was that you'd first swap X1600/X1830, then add support for hardware
> dual-edge support if >= X1600, and finally add X2600 support.

Ah, ok. Fine with that. Will do for V2.

BR and thanks,
Nikolaus

> 
>>>> The corrected interrupt configuration has been tested on X1600,
>>>> X2000, and X2600, including GPIO dual-edge interrupts by a
>>>> gpio-button.
>>>> 
>>>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>>>> ---
>>>> drivers/pinctrl/pinctrl-ingenic.c | 71 +++++++++++++++++++++++++------
>>>> 1 file changed, 58 insertions(+), 13 deletions(-)
>>>> 
>>>> diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-
>>>> ingenic.c
>>>> index a62532c49fb60..90c734c74d267 100644
>>>> --- a/drivers/pinctrl/pinctrl-ingenic.c
>>>> +++ b/drivers/pinctrl/pinctrl-ingenic.c
>>>> @@ -70,6 +70,10 @@
>>>> #define X2000_GPIO_SR 0xd0
>>>> #define X2000_GPIO_SMT 0xe0
>>>> 
>>>> +#define X2600_GPIO_FLAG_CLR 0x58 /* write only */
>>>> +#define X2600_GPIO_SHADOW_OFFSET 0x800
>>>> +#define X2600_GPIO_PAGLD (X2600_GPIO_SHADOW_OFFSET +
>>>> 0xf0)
>>>> +
>>>> #define REG_SET(x) ((x) + 0x4)
>>>> #define REG_CLEAR(x) ((x) + 0x8)
>>>> 
>>>> @@ -114,6 +118,7 @@ enum jz_version {
>>>>  ID_X1830,
>>>>  ID_X2000,
>>>>  ID_X2100,
>>>> + ID_X2600,
>>>> };
>>>> 
>>>> struct ingenic_chip_info {
>>>> @@ -164,7 +169,8 @@ static const unsigned long enabled_socs =
>>>>  IS_ENABLED(CONFIG_MACH_X1600) << ID_X1600 |
>>>>  IS_ENABLED(CONFIG_MACH_X1830) << ID_X1830 |
>>>>  IS_ENABLED(CONFIG_MACH_X2000) << ID_X2000 |
>>>> - IS_ENABLED(CONFIG_MACH_X2100) << ID_X2100;
>>>> + IS_ENABLED(CONFIG_MACH_X2100) << ID_X2100 |
>>>> + IS_ENABLED(CONFIG_MACH_X2600) << ID_X2600;
>>>> 
>>>> static bool
>>>> is_soc_or_above(const struct ingenic_pinctrl *jzpc, enum jz_version
>>>> version)
>>>> @@ -3545,15 +3551,21 @@ static void ingenic_gpio_shadow_set_bit(struct
>>>> ingenic_gpio_chip *jzgc,
>>>>  else
>>>>  reg = REG_CLEAR(reg);
>>>> 
>>>> - regmap_write(jzgc->jzpc->map, REG_PZ_BASE(
>>>> - jzgc->jzpc->info->reg_offset) + reg, BIT(offset));
>>>> + if (is_soc_or_above(jzgc->jzpc, ID_X2600))
>>>> + regmap_write(jzgc->jzpc->map, jzgc->reg_base +
>>>> X2600_GPIO_SHADOW_OFFSET + reg,
>>>> +      BIT(offset));
>>>> + else
>>>> + regmap_write(jzgc->jzpc->map, REG_PZ_BASE(jzgc->jzpc->info-
>>>>> reg_offset) + reg,
>>>> +      BIT(offset));
>>>> }
>>>> 
>>>> static void ingenic_gpio_shadow_set_bit_load(struct ingenic_gpio_chip
>>>> *jzgc)
>>>> {
>>>> - regmap_write(jzgc->jzpc->map, REG_PZ_GID2LD(
>>>> - jzgc->jzpc->info->reg_offset),
>>>> - jzgc->gc.base / PINS_PER_GPIO_CHIP);
>>>> + if (is_soc_or_above(jzgc->jzpc, ID_X2600))
>>>> + regmap_write(jzgc->jzpc->map, jzgc->reg_base +
>>>> X2600_GPIO_PAGLD, 1);
>>>> + else
>>>> + regmap_write(jzgc->jzpc->map, REG_PZ_GID2LD(jzgc->jzpc->info-
>>>>> reg_offset),
>>>> +      jzgc->gc.base / PINS_PER_GPIO_CHIP);
>>>> }
>>>> 
>>>> static void jz4730_gpio_set_bits(struct ingenic_gpio_chip *jzgc,
>>>> @@ -3618,6 +3630,12 @@ static void irq_set_type(struct ingenic_gpio_chip
>>>> *jzgc,
>>>>  break;
>>>>  }
>>>> 
>>>> + if (type == IRQ_TYPE_EDGE_BOTH &&
>>>> +     (is_soc_or_above(jzgc->jzpc, ID_X2000) ||
>>>> +      (is_soc_or_above(jzgc->jzpc, ID_X1600) &&
>>>> +       !is_soc_or_above(jzgc->jzpc, ID_X1830))))
>>> 
>>> I remember you saying (in another patchset) that the X1830 was older than
>>> the
>>> X1600, if that's the case the IDs for X1600 and X1830 should be swapped. I
>>> believe this patch would then become much simpler.
>> 
>> Indeed. So I can swap the ID's in V2.
> 
> I just checked in the programming manuals and indeed it seems to be the case.
> Weird! I had no idea.
> 
> Cheers,
> -Paul
> 
>>>> + val2 = true;
>>>> +
>>>>  if (is_soc_or_above(jzgc->jzpc, ID_JZ4770)) {
>>>>  reg1 = JZ4770_GPIO_PAT1;
>>>>  reg2 = JZ4770_GPIO_PAT0;
>>>> @@ -3640,6 +3658,9 @@ static void irq_set_type(struct ingenic_gpio_chip
>>>> *jzgc,
>>>>  ingenic_gpio_shadow_set_bit(jzgc, reg2, offset, val1);
>>>>  ingenic_gpio_shadow_set_bit(jzgc, reg1, offset, val2);
>>>>  ingenic_gpio_shadow_set_bit_load(jzgc);
>>>> +
>>>> + if (is_soc_or_above(jzgc->jzpc, ID_X1600) &&
>>>> !is_soc_or_above(jzgc->jzpc, ID_X1830))
>>>> + ingenic_gpio_set_bit(jzgc, X2000_GPIO_EDG, offset,
>>>> val3);
>>>>  } else {
>>>>  ingenic_gpio_set_bit(jzgc, reg2, offset, val1);
>>>>  ingenic_gpio_set_bit(jzgc, reg1, offset, val2);
>>>> @@ -3714,7 +3735,8 @@ static void ingenic_gpio_irq_ack(struct irq_data
>>>> *irqd)
>>>>  bool high;
>>>> 
>>>>  if ((irqd_get_trigger_type(irqd) == IRQ_TYPE_EDGE_BOTH) &&
>>>> -     !is_soc_or_above(jzgc->jzpc, ID_X2000)) {
>>>> +     (!is_soc_or_above(jzgc->jzpc, ID_X1600) ||
>>>> +     (is_soc_or_above(jzgc->jzpc, ID_X1830) && !is_soc_or_above(jzgc-
>>>>> jzpc, ID_X2000)))) {
>>>>  /*
>>>>   * Switch to an interrupt for the opposite edge to the one
>>>> that
>>>>   * triggered the interrupt being ACKed.
>>>> @@ -3726,7 +3748,9 @@ static void ingenic_gpio_irq_ack(struct irq_data
>>>> *irqd)
>>>>  irq_set_type(jzgc, irq, IRQ_TYPE_LEVEL_HIGH);
>>>>  }
>>>> 
>>>> - if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
>>>> + if (is_soc_or_above(jzgc->jzpc, ID_X2600))
>>>> + regmap_write(jzgc->jzpc->map, jzgc->reg_base +
>>>> X2600_GPIO_FLAG_CLR, BIT(irq));
>>>> + else if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
>>>>  ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_FLAG, irq, false);
>>>>  else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
>>>>  ingenic_gpio_set_bit(jzgc, JZ4740_GPIO_DATA, irq, true);
>>>> @@ -3754,7 +3778,9 @@ static int ingenic_gpio_irq_set_type(struct irq_data
>>>> *irqd, unsigned int type)
>>>>  irq_set_handler_locked(irqd, handle_bad_irq);
>>>>  }
>>>> 
>>>> - if ((type == IRQ_TYPE_EDGE_BOTH) && !is_soc_or_above(jzgc->jzpc,
>>>> ID_X2000)) {
>>>> + if (type == IRQ_TYPE_EDGE_BOTH &&
>>>> +     (!is_soc_or_above(jzgc->jzpc, ID_X1600) ||
>>>> +     (is_soc_or_above(jzgc->jzpc, ID_X1830) && !is_soc_or_above(jzgc-
>>>>> jzpc, ID_X2000)))) {
>>>>  /*
>>>>   * The hardware does not support interrupts on both edges.
>>>> The
>>>>   * best we can do is to set up a single-edge interrupt and
>>>> then
>>>> @@ -3849,16 +3875,35 @@ static inline void
>>>> ingenic_shadow_config_pin(struct
>>>> ingenic_pinctrl *jzpc,
>>>>  unsigned int pin, u8 reg, bool set)
>>>> {
>>>>  unsigned int idx = pin % PINS_PER_GPIO_CHIP;
>>>> + unsigned int offt = pin / PINS_PER_GPIO_CHIP;
>>>> 
>>>> - regmap_write(jzpc->map, REG_PZ_BASE(jzpc->info->reg_offset) +
>>>> - (set ? REG_SET(reg) : REG_CLEAR(reg)), BIT(idx));
>>>> + if (set)
>>>> + reg = REG_SET(reg);
>>>> + else
>>>> + reg = REG_CLEAR(reg);
>>>> +
>>>> + if (is_soc_or_above(jzpc, ID_X2600)) {
>>>> + u32 target_reg = (offt * jzpc->info->reg_offset) +
>>>> X2600_GPIO_SHADOW_OFFSET + reg;
>>>> +
>>>> + regmap_write(jzpc->map, target_reg, BIT(idx));
>>>> + } else {
>>>> + regmap_write(jzpc->map, REG_PZ_BASE(jzpc->info->reg_offset) +
>>>> reg, BIT(idx));
>>>> + }
>>>> }
>>>> 
>>>> static inline void ingenic_shadow_config_pin_load(struct ingenic_pinctrl
>>>> *jzpc,
>>>>  unsigned int pin)
>>>> {
>>>> - regmap_write(jzpc->map, REG_PZ_GID2LD(jzpc->info->reg_offset),
>>>> - pin / PINS_PER_GPIO_CHIP);
>>>> + unsigned int offt = pin / PINS_PER_GPIO_CHIP;
>>>> +
>>>> + if (is_soc_or_above(jzpc, ID_X2600)) {
>>>> + u32 target_load_reg = (offt * jzpc->info->reg_offset) +
>>>> X2600_GPIO_PAGLD;
>>>> +
>>>> + regmap_write(jzpc->map, target_load_reg, 1);
>>>> + } else {
>>>> + regmap_write(jzpc->map, REG_PZ_GID2LD(jzpc->info-
>>>>> reg_offset),
>>>> +      pin / PINS_PER_GPIO_CHIP);
>>>> + }
>>>> }
>>>> 
>>>> static inline void jz4730_config_pin_function(struct ingenic_pinctrl
>>>> *jzpc,