Hi Lukas,
On 5/19/26 6:32 PM, Lukas Timmermann wrote:
[You don't often get email from [email protected]. Learn why this is
important at https://aka.ms/LearnAboutSenderIdentification ]
On Wed, May 06, 2026 at 03:14:11PM +0200, Quentin Schulz wrote:
Hi Lukas,
On 5/6/26 1:13 AM, Lukas Timmermann wrote:
This patch series adds the necessary files to use a modern pinctrl driver
with an exynos5250 SoC. The changes are well tested and are working.
In contrast to v1, this reenables GPIO functionality. But as I'm still
lacking experience, I'm not quite sure on how to prevent s5p_gpio to
load when using upstream DTS in a clean way. Because of that, this
patch series is marked as an RFC.
checkpatch complains about using #ifdef.
We already have an issue with samsung,exynos78x0-gpio so we'd need a similar
work-around for that one.
An easy way to sort this out would be to know if we know of devices which
are still using the old binding? Qualcomm is rather special, is Samsung
Exynos also special with regard to how it handles Device Trees? Do we get it
from an earlier boot stage or is everything coming from U-Boot itself? If
the latter, we can always migrate the existing devices to the new binding
and remove support for the old binding and with it the same compatible in
two drivers. Another question would be, does it make sense for both drivers
to be compiled? If not (e.g. it's only pinctrl IPs inside an SoC and there's
only one pinctrl device per SoC, so if your device supports the new binding,
you only need the new driver). Then we can add a
depends on !PINCTRL_EXYNOS5250
for CONFIG_S5P (we probably should have a different symbol for the GPIO
driver though, maybe CONFIG_GPIO_S5P?).
Cheers,
Quentin
I'm sorry. Some of these Infos should've been in my original message:
This driver replaces the pinctrl functionality in the old s5p_gpio
I don't see how s5p_gpio is currently doing pinctrl. Do you rather mean
that the s5p_gpio driver is matching the pinctrl compatible and thus we
cannot have two drivers match the same compatible?
driver. It doesn't replace GPIO functionality itself. So both drivers
have to be compiled in order to work with modern upstream DTs. This
driver binds the older s5p_gpio driver during its own binding.
The pinctrl dt-bindings are incompatible to each other. This driver fixes
Can you highlight how they are incompatible? I think the pinctrl part is
identical?
arch/arm/dts/exynos5250.dtsi
/ {
[...]
pinctrl_0: pinctrl@11400000 {
compatible = "samsung,exynos5250-pinctrl";
reg = <0x11400000 0x1000>;
interrupts = <0 46 0>;
wakup_eint: wakeup-interrupt-controller {
compatible = "samsung,exynos4210-wakeup-eint";
interrupt-parent = <&gic>;
interrupts = <0 32 0>;
};
};
pinctrl_1: pinctrl@13400000 {
compatible = "samsung,exynos5250-pinctrl";
reg = <0x13400000 0x1000>;
interrupts = <0 45 0>;
};
pinctrl_2: pinctrl@10d10000 {
compatible = "samsung,exynos5250-pinctrl";
reg = <0x10d10000 0x1000>;
interrupts = <0 50 0>;
};
pinctrl_3: pinctrl@03860000 {
compatible = "samsung,exynos5250-pinctrl";
reg = <0x03860000 0x1000>;
interrupts = <0 47 0>;
};
[...]
};
+ arch/arm/dts/exynos5250-pinctrl.dtsi
/ {
pinctrl@11400000 {
gpa0: gpa0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpa1: gpa1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpa2: gpa2 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpb0: gpb0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpb1: gpb1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpb2: gpb2 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpb3: gpb3 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpc0: gpc0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpc1: gpc1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpc2: gpc2 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpc3: gpc3 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpd0: gpd0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpd1: gpd1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpy0: gpy0 {
gpio-controller;
#gpio-cells = <2>;
};
gpy1: gpy1 {
gpio-controller;
#gpio-cells = <2>;
};
gpy2: gpy2 {
gpio-controller;
#gpio-cells = <2>;
};
gpy3: gpy3 {
gpio-controller;
#gpio-cells = <2>;
};
gpy4: gpy4 {
gpio-controller;
#gpio-cells = <2>;
};
gpy5: gpy5 {
gpio-controller;
#gpio-cells = <2>;
};
gpy6: gpy6 {
gpio-controller;
#gpio-cells = <2>;
};
gpc4: gpc4 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpx0: gpx0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
interrupt-parent = <&combiner>;
#interrupt-cells = <2>;
interrupts = <23 0>, <24 0>, <25 0>, <25 1>,
<26 0>, <26 1>, <27 0>, <27 1>;
};
gpx1: gpx1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
interrupt-parent = <&combiner>;
#interrupt-cells = <2>;
interrupts = <28 0>, <28 1>, <29 0>, <29 1>,
<30 0>, <30 1>, <31 0>, <31 1>;
};
gpx2: gpx2 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpx3: gpx3 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
};
pinctrl@13400000 {
gpe0: gpe0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpe1: gpe1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpf0: gpf0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpf1: gpf1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpg0: gpg0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpg1: gpg1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpg2: gpg2 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gph0: gph0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gph1: gph1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
};
pinctrl@10d10000 {
gpv0: gpv0 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpv1: gpv1 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpv2: gpv2 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpv3: gpv3 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpv4: gpv4 {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
};
pinctrl@03860000 {
gpz: gpz {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
};
};
vs dts/upstream/src/arm/samsung/exynos5250.dtsi
/{
[...]
pinctrl_0: pinctrl@11400000 {
compatible = "samsung,exynos5250-pinctrl";
reg = <0x11400000 0x1000>;
interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
wakup_eint: wakeup-interrupt-controller {
compatible =
"samsung,exynos4210-wakeup-eint";
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 32
IRQ_TYPE_LEVEL_HIGH>;
};
};
pinctrl_1: pinctrl@13400000 {
compatible = "samsung,exynos5250-pinctrl";
reg = <0x13400000 0x1000>;
interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
};
pinctrl_2: pinctrl@10d10000 {
compatible = "samsung,exynos5250-pinctrl";
reg = <0x10d10000 0x1000>;
interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
};
pinctrl_3: pinctrl@3860000 {
compatible = "samsung,exynos5250-pinctrl";
reg = <0x03860000 0x1000>;
interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
power-domains = <&pd_mau>;
};
+ dts/upstream/src/arm/samsung/exynos5250-pinctrl.dtsi
&pinctrl_0 {
gpa0: gpa0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpa1: gpa1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpa2: gpa2-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpb0: gpb0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpb1: gpb1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpb2: gpb2-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpb3: gpb3-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpc0: gpc0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpc1: gpc1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpc2: gpc2-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpc3: gpc3-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpd0: gpd0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpd1: gpd1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpy0: gpy0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
};
gpy1: gpy1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
};
gpy2: gpy2-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
};
gpy3: gpy3-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
};
gpy4: gpy4-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
};
gpy5: gpy5-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
};
gpy6: gpy6-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
};
gpc4: gpc4-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpx0: gpx0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
interrupt-parent = <&combiner>;
#interrupt-cells = <2>;
interrupts = <23 0>, <24 0>, <25 0>, <25 1>,
<26 0>, <26 1>, <27 0>, <27 1>;
};
gpx1: gpx1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
interrupt-parent = <&combiner>;
#interrupt-cells = <2>;
interrupts = <28 0>, <28 1>, <29 0>, <29 1>,
<30 0>, <30 1>, <31 0>, <31 1>;
};
gpx2: gpx2-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpx3: gpx3-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
[...]
};
&pinctrl_1 {
gpe0: gpe0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpe1: gpe1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpf0: gpf0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpf1: gpf1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpg0: gpg0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpg1: gpg1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpg2: gpg2-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gph0: gph0-gpio-bank {
gpio-controller;
#gpio-cells = <2>
interrupt-controller;
#interrupt-cells = <2>;
};
gph1: gph1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
[...]
};
&pinctrl_2 {
gpv0: gpv0-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpv1: gpv1-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpv2: gpv2-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpv3: gpv3-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
gpv4: gpv4-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
[...]
};
&pinctrl_3 {
gpz: gpz-gpio-bank {
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
};
[...]
};
I can see the interrupts property in the pinctrl@ node is 0 vs 4
(IRQ_TYPE_LEVEL_HIGH) and the node name is slightly different (I don't
think it matters).
The pinctrl nodes are also in the root (/) node in U-Boot, while in
upstream DTS it's under /soc (don't think it matters provided something
takes care of probing /soc and its children?).
The gpio nodes have an additional -gpio-bank suffix in upstream DTS,
don't think it matters either.
There are additional pinconf/pinmux/pingroups (I believe?) nodes but
those don't have a gpio-controller property so they shouldn't matter?
I must be missing something, can you tell me what?
pinctrl with upstream DTs and the older one must be used on it's own
when relying on U-Boots deprecated DTs.
DTs are provided by U-Boot itself, at least that's the case on my device
(exynos5250-manta).
If I understood you correctly, we could modify the deprecated exynos5250.dtsi
to fix this situation. But I don't know about possible side effects
and I'm not that experienced in kernel/bootloader development. Is this an
option?
I had split the gpio symbol into it's own thing before and could add that
back into v4 but it's functionally the same. (Both drivers needed for
upstream DT)
No, I think it's fine to have pinctrl and GPIO split like you suggested
here.
Can you explain what makes your pinctrl driver require the upstream
Device Tree (the one in dts/) and cannot work with the U-Boot device
tree (the one in arch/arm/dts)?
We have a problem if it's requiring an upstream Device Tree because your
new driver doesn't enforce it (via a depends on OF_UPSTREAM in Kconfig),
and we have one if it doesn't, because the ifdefery in s5p_gpio is then
incorrect as you can compile with CONFIG_S5P AND
CONFIG_PINCTRL_EXYNOS5250 with both trying to bind against the same
compatible.
I think what you want in the second patch (provided I'm right the
pinctrl driver can work with the U-Boot DTS) is instead:
#if CONFIG_IS_ENABLED(PINCTRL) && IS_ENABLED(CONFIG_PINCTRL_EXYNOS5250)
{ .compatible = "samsung,exynos5250-pinctrl" },
#endif
and add a nice comment around those lines to say that we do this because
drivers/pinctrl/exynos/pinctrl-exynos5250.c binds against the same
compatible and will also bind s5p_gpio against the same node.
Otherwise, my suggestion was to maybe remove the compatible entirely
from s5p_gpio. For that, you'd need to make sure the new driver is
compatible with the device tree they are using (I'm assuming U-Boot
device tree and not upstream) and you'd need to enable CONFIG_PINCTRL
(and possibly CONFIG_SPL/TPL_PINCTRL) + CONFIG_EXYNOS5250 for defconfigs
where CONFIG_S5P is enabled (and in all xPL/proper phases, depending on
which CONFIG_GPIO and CONFIG_SPL/TPL_GPIO is enabled).
Maybe there's also a path to migrate the four Exynos5250 defconfigs to
use upstream Device Tree but one would need to compare the current
U-Boot device tree we have and the upstream one (in dts/) and check that
all drivers are compatible with the upstream device tree before
switching them. Not a requirement for your series, but it'd be nice as a
follow if this is something I can convince you into having a look :)
Cheers,
Quentin
