Re: [PATCH 1/5] pinctrl: sunxi: Fix a100 emac pin function name
From: Chen-Yu Tsai <hidden>
Date: 2025-07-07 10:22:40
Also in:
linux-devicetree, linux-gpio, linux-sunxi, lkml, netdev
On Mon, Jul 7, 2025 at 6:13 PM Paul Kocialkowski [off-list ref] wrote:
Hi, Le Mon 07 Jul 25, 17:52, Chen-Yu Tsai a écrit :quoted
On Mon, Jul 7, 2025 at 5:39 PM Paul Kocialkowski [off-list ref] wrote:quoted
Hi Chen-Yu, Le Sun 06 Jul 25, 23:04, Chen-Yu Tsai a écrit :quoted
On Sun, Jul 6, 2025 at 8:00 AM Paul Kocialkowski [off-list ref] wrote:quoted
Hi Andre, Le Sat 05 Jul 25, 15:38, Andre Przywara a écrit :quoted
On Fri, 4 Jul 2025 23:35:35 +0100 Andre Przywara [off-list ref] wrote: Hi,quoted
On Thu, 26 Jun 2025 10:09:19 +0200 Paul Kocialkowski [off-list ref] wrote: Hi Paul,quoted
The Allwinner A100/A133 only has a single emac instance, which is referred to as "emac" everywhere. Fix the pin names to drop the trailing "0" that has no reason to be.Sorry, but this is wrong. There *is* a second EMAC on the A133 die: it's indeed not mentioned in the manual, but you can probe its MMIO registers (@0x5030000), and there is a second syscon register (@0x03000034). It's mentioned in several BSP code places ([1]). It seem like no suitable pins are connected on the A133 package, but that should not affect the A100 .dtsi (we use a similar approach for the H616 and A523). So I think we should keep the emac0 name.just thinking that it's even worse: this changes the DT visible pinctrl function name, so it's a DT ABI change. With the "emac0" function name, Ethernet would work with stable kernels already (as everything is compatible, it's just about DT changes). But with this change, pinctrl drivers in older kernels would not match.Given that the port is still very early and experimental and has very few users and no field deployment so I don't really think it would have annoyed anybody in practice. But yes in principle you are right, while the header renames keep the same value, the string names are used to match the device-tree definitions and this constitues ABI that needs to remain stable.quoted
So I would very much like to see this patch moved out. Is it just in LinusW's tree so far? I don't see it in -next yet.I don't think the patches were accepted for over a week so we can probably still act. I will send reverts, unless maintainers want to manually remove these commits?I can drop the dts patches from the sunxi tree. Linus might be able to drop the pinctrl patch. You definitely need to send a revert for the DT binding patch that is already in net-next.Should this really affect the bindings though? From what Andre reported, both EMAC0 and EMAC1 should be the same block so it doesn't seem particularly necessary to have a different compatible. Looking at Allwiner's BSP code for the A133[0], I don't see any difference between the two. While there's device_type property in Allwinner's dt, it's apparently not used by the driver[1]. So I think we're still fine with a single compatible (without the controller index in it).The block is the same, but the integration is slightly different, as the register for the RGMII clock delays and other stuff is at a different offset in the system controller. The BSP handles this by directly including the register in the "reg" property.Ah I see, I forgot about the syscon register. However it doesn't seem like a very good approach to have a different compatible to express the idea that an external resource is different. Just like we do for clocks, resets and other things, we should probably find a way to express the offset via some dedicated property instead of spinning a different compatible each time it changes.quoted
So yes, you do need a separate compatible string, if only to deal with the slight difference in the integration layer.So maybe an additional allwinner,syscon-offset property or a new
If you can get that accepted, I think that works?
allwinner,syscon that takes the syscon phandle first and the offset second?
I would prefer to avoid any changes to the syscon reference that would require more custom code. I only just recently found that we could use the standard syscon code with the provider registering the syscon. We could drop the of_parse_phandle() + find device + dev_get_regmap() bits. This is partially covered in my GMAC200 series. ChenYu
It seems that various other platforms are doing similar things (e.g. ti,syscon-pcie-mode). Thanks Paulquoted
ChenYuquoted
[0]: https://github.com/engSinteck/A133_Image/blob/main/longan/kernel/linux-4.9/arch/arm64/boot/dts/sunxi/sun50iw10p1.dtsi#L2016 [1]: https://github.com/engSinteck/A133_Image/blob/main/longan/kernel/linux-4.9/drivers/net/ethernet/allwinner/sunxi-gmac.c All the best, Paulquoted
ChenYuquoted
Cheers, Paulquoted
Cheers, Andre.quoted
[1] https://github.com/qiaoweibiao/T507_Kernel/blob/main/arch/arm64/boot/dts/sunxi/sun50iw10p1.dtsiquoted
Fixes: 473436e7647d ("pinctrl: sunxi: add support for the Allwinner A100 pin controller") Signed-off-by: Paul Kocialkowski <paulk@sys-base.io> --- drivers/pinctrl/sunxi/pinctrl-sun50i-a100.c | 32 ++++++++++----------- 1 file changed, 16 insertions(+), 16 deletions(-)diff --git a/drivers/pinctrl/sunxi/pinctrl-sun50i-a100.c b/drivers/pinctrl/sunxi/pinctrl-sun50i-a100.c index b97de80ae2f3..95b764ee1c0d 100644 --- a/drivers/pinctrl/sunxi/pinctrl-sun50i-a100.c +++ b/drivers/pinctrl/sunxi/pinctrl-sun50i-a100.c@@ -546,33 +546,33 @@ static const struct sunxi_desc_pin a100_pins[] = { SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x2, "i2c0"), /* SCK */ - SUNXI_FUNCTION(0x5, "emac0"), /* RXD1 */ + SUNXI_FUNCTION(0x5, "emac"), /* RXD1 */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 0)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 1), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x2, "i2c0"), /* SDA */ - SUNXI_FUNCTION(0x5, "emac0"), /* RXD0 */ + SUNXI_FUNCTION(0x5, "emac"), /* RXD0 */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 1)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 2), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x2, "i2c1"), /* SCK */ - SUNXI_FUNCTION(0x5, "emac0"), /* RXCTL */ + SUNXI_FUNCTION(0x5, "emac"), /* RXCTL */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 2)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 3), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x2, "i2c1"), /* SDA */ SUNXI_FUNCTION(0x3, "cir0"), /* OUT */ - SUNXI_FUNCTION(0x5, "emac0"), /* CLKIN */ + SUNXI_FUNCTION(0x5, "emac"), /* CLKIN */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 3)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 4), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x2, "uart3"), /* TX */ SUNXI_FUNCTION(0x3, "spi1"), /* CS */ - SUNXI_FUNCTION(0x5, "emac0"), /* TXD1 */ + SUNXI_FUNCTION(0x5, "emac"), /* TXD1 */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 4)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 5), SUNXI_FUNCTION(0x0, "gpio_in"),@@ -580,14 +580,14 @@ static const struct sunxi_desc_pin a100_pins[] = { SUNXI_FUNCTION(0x2, "uart3"), /* RX */ SUNXI_FUNCTION(0x3, "spi1"), /* CLK */ SUNXI_FUNCTION(0x4, "ledc"), - SUNXI_FUNCTION(0x5, "emac0"), /* TXD0 */ + SUNXI_FUNCTION(0x5, "emac"), /* TXD0 */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 5)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 6), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x2, "uart3"), /* RTS */ SUNXI_FUNCTION(0x3, "spi1"), /* MOSI */ - SUNXI_FUNCTION(0x5, "emac0"), /* TXCK */ + SUNXI_FUNCTION(0x5, "emac"), /* TXCK */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 6)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 7), SUNXI_FUNCTION(0x0, "gpio_in"),@@ -595,7 +595,7 @@ static const struct sunxi_desc_pin a100_pins[] = { SUNXI_FUNCTION(0x2, "uart3"), /* CTS */ SUNXI_FUNCTION(0x3, "spi1"), /* MISO */ SUNXI_FUNCTION(0x4, "spdif"), /* OUT */ - SUNXI_FUNCTION(0x5, "emac0"), /* TXCTL */ + SUNXI_FUNCTION(0x5, "emac"), /* TXCTL */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 7)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 8), SUNXI_FUNCTION(0x0, "gpio_in"),@@ -611,7 +611,7 @@ static const struct sunxi_desc_pin a100_pins[] = { SUNXI_FUNCTION(0x2, "dmic"), /* DATA0 */ SUNXI_FUNCTION(0x3, "spi2"), /* CLK */ SUNXI_FUNCTION(0x4, "i2s2"), /* BCLK */ - SUNXI_FUNCTION(0x5, "emac0"), /* MDC */ + SUNXI_FUNCTION(0x5, "emac"), /* MDC */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 9)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 10), SUNXI_FUNCTION(0x0, "gpio_in"),@@ -619,7 +619,7 @@ static const struct sunxi_desc_pin a100_pins[] = { SUNXI_FUNCTION(0x2, "dmic"), /* DATA1 */ SUNXI_FUNCTION(0x3, "spi2"), /* MOSI */ SUNXI_FUNCTION(0x4, "i2s2"), /* LRCK */ - SUNXI_FUNCTION(0x5, "emac0"), /* MDIO */ + SUNXI_FUNCTION(0x5, "emac"), /* MDIO */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 10)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 11), SUNXI_FUNCTION(0x0, "gpio_in"),@@ -642,33 +642,33 @@ static const struct sunxi_desc_pin a100_pins[] = { SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x3, "i2c3"), /* SCK */ SUNXI_FUNCTION(0x4, "i2s3"), /* MCLK */ - SUNXI_FUNCTION(0x5, "emac0"), /* EPHY */ + SUNXI_FUNCTION(0x5, "emac"), /* EPHY */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 13)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 14), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x4, "i2s3"), /* BCLK */ - SUNXI_FUNCTION(0x5, "emac0"), /* RXD3 */ + SUNXI_FUNCTION(0x5, "emac"), /* RXD3 */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 14)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 15), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x4, "i2s3"), /* LRCK */ - SUNXI_FUNCTION(0x5, "emac0"), /* RXD2 */ + SUNXI_FUNCTION(0x5, "emac"), /* RXD2 */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 15)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 16), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x3, "i2s3_dout0"), /* DOUT0 */ SUNXI_FUNCTION(0x4, "i2s3_din1"), /* DIN1 */ - SUNXI_FUNCTION(0x5, "emac0"), /* RXCK */ + SUNXI_FUNCTION(0x5, "emac"), /* RXCK */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 16)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 17), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x3, "i2s3_dout1"), /* DOUT1 */ SUNXI_FUNCTION(0x4, "i2s3_din0"), /* DIN0 */ - SUNXI_FUNCTION(0x5, "emac0"), /* TXD3 */ + SUNXI_FUNCTION(0x5, "emac"), /* TXD3 */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 17)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 18), SUNXI_FUNCTION(0x0, "gpio_in"),@@ -676,7 +676,7 @@ static const struct sunxi_desc_pin a100_pins[] = { SUNXI_FUNCTION(0x2, "cir0"), /* OUT */ SUNXI_FUNCTION(0x3, "i2s3_dout2"), /* DOUT2 */ SUNXI_FUNCTION(0x4, "i2s3_din2"), /* DIN2 */ - SUNXI_FUNCTION(0x5, "emac0"), /* TXD2 */ + SUNXI_FUNCTION(0x5, "emac"), /* TXD2 */ SUNXI_FUNCTION_IRQ_BANK(0x6, 6, 18)), SUNXI_PIN(SUNXI_PINCTRL_PIN(H, 19), SUNXI_FUNCTION(0x0, "gpio_in"),-- Paul Kocialkowski, Independent contractor - sys-base - https://www.sys-base.io/ Free software developer - https://www.paulk.fr/ Expert in multimedia, graphics and embedded hardware support with Linux.-- Paul Kocialkowski, Independent contractor - sys-base - https://www.sys-base.io/ Free software developer - https://www.paulk.fr/ Expert in multimedia, graphics and embedded hardware support with Linux.-- Paul Kocialkowski, Independent contractor - sys-base - https://www.sys-base.io/ Free software developer - https://www.paulk.fr/ Expert in multimedia, graphics and embedded hardware support with Linux.