rockchip rk3328 pinctrl unable to change gpio function of pins defined in rk3328_mux_recalced_data

rockchip rk3328 pinctrl unable to change gpio function of pins defined in rk3328_mux_recalced_data

[Kernel.org Bugbot]

antwain.schneider writes via Kernel.org Bugzilla:

rock64 exposes i2s1 on a secondary 22 pin header on the machine, this includes gpio2b7
since i don't care about i2s and those pins would be more valuable as gpios, i explicitly disabled i2s1 in the device tree
i set gpio2b7 as a cs-gpio but it wasn't working, eventually i got bold and actually started looking at the grf in memory

ma -k /dev/gpiomem dd 0x28 4
00000020  -------- --------  00000001

no matter how many i2s1 things i disabled or removed from the dtb, GRF_GPIO2BH_IOMUX always read 00000001, meaning i2s1_mclk seems hardcoded
eventually i found the description in commit 3818e4a7678ea70508b6b6e72a4cbf445e1e7dfa and realized gpio2b4 also gives me trouble
i feel this is a bug, but probably not important for a 6 year old chip

View: https://bugzilla.kernel.org/show_bug.cgi?id=217334#c0
You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: rockchip rk3328 pinctrl unable to change gpio function of pins defined in rk3328_mux_recalced_data

[Kernel.org Bugbot]

antwain.schneider writes via Kernel.org Bugzilla:

so here's as best as i understand what's going on

in pinctrl-rockchip.c

rk3328_pin_banks

PIN_BANK_IOMUX_FLAGS(2, 32, "gpio2", 0, IOMUX_WIDTH_3BIT, IOMUX_WIDTH_3BIT, 0),

it states that gpio2b is 3 bits per gpio, but if you look at the rk3328 trm, it's only 1 gpio pin in bank 2 that is three bits, e.g.

/* GRF_GPIO2BL_IOMUX 2'bxxx */
/* 2b0 */	"spi_clkm0", NULL, NULL,
/* 2b1 */	"spi_txdm0", NULL, NULL,
/* 2b2 */	"spi_rxdm0", NULL, NULL,
/* 2b3 */	"spi_csn0m0", NULL, NULL,
/* 2b4 */	"spi_csn1m0", "flash_vol_sel", NULL,
/* 2b5 */	"i2c2_sda", "tsadc_shut", NULL,
/* 2b6 */	"i2c2_scl", NULL, NULL,
/* GRF_GPIO2BH_IOMUX 3'bxxx */
/* 2b7 */	"i2s1_mclk", NULL, "tsp_syncm1", "cif_clkoutm1", NULL, NULL, NULL,

so by not adding addtional explicit 2 bit alignment adjustments to rk3328_mux_recalced_data, everything is 'slightly off'

david wu originally submitted the correct alignment on the initial patch[1], but it wasn't commited, but on the official rockchip linux fork, he readded the alignment in 2019[2]

so i request someone please add this back into the kernel

[1] https://lore.kernel.org/linux-rockchip/1485074286-7863-1-git-send-email-david.wu@rock-chips.com/
[2] https://github.com/rockchip-linux/kernel/commit/d69af8ab6534bb28c1556076f08d2a5ab4935d95

View: https://bugzilla.kernel.org/show_bug.cgi?id=217334#c1
You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: rockchip rk3328 pinctrl unable to change gpio function of pins defined in rk3328_mux_recalced_data

[Kernel.org Bugbot]

antwain.schneider added an attachment on Kernel.org Bugzilla:

Created attachment 304169
the other half

missed from the 2019 patch is the recalculations for the gpio3b, e.g.

/* GRF_GPIO3BL_IOMUX 3'bxxx */
/* 3b0 */	"tsp_d4", "cif_data4", "spi_csn0m2", "i2s2_lrcktxm1", "usb3phy_debug8", "i2s2_lrckrxm1", NULL,
/* GRF_GPIO3BH_IOMUX 2'bxx */
/* 3b1 */	"tsp_d5m0", "cif_data5m", NULL,
/* 3b2 */	"tsp_d6m0", "cif_data6m", NULL,
/* 3b3 */	"tsp_d7m0", "cif_data7m", NULL,
/* 3b4 */	"card_clkm0", NULL, NULL,
/* 3b5 */	"card_rstm0", NULL, NULL,
/* 3b6 */	"card_detm0", NULL, NULL,
/* 3b7 */	"card_iom0", NULL, NULL,

so attached is the missing half of the initial correct recalculations in the modern format

File: rockchip-rk3328-pinctrl-gpio3b-recalc.diff (text/plain)
Size: 743.00 B
Link: https://bugzilla.kernel.org/attachment.cgi?id=304169
---
the other half

You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: rockchip rk3328 pinctrl unable to change gpio function of pins defined in rk3328_mux_recalced_data

[Kernel.org Bugbot]

antwain.schneider added an attachment on Kernel.org Bugzilla:

Created attachment 304170
correct other half

sorry, off by 8 error :O

File: rockchip-rk3328-pinctrl-gpio3b-recalc.diff (text/plain)
Size: 749.00 B
Link: https://bugzilla.kernel.org/attachment.cgi?id=304170
---
correct other half

You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: rockchip rk3328 pinctrl overflow

[Kernel.org Bugbot]

antwain.schneider writes via Kernel.org Bugzilla:

to try to make it clearer, gpio2b is marked as IOMUX_WIDTH_3BIT

GRF_GPIO2BL_IOMUX
2b0  2b1  2b2  2b3  2b4  2b5  2b6
[xxx][xxx][xxx][xxx][xxx][xxx][xxx] 21 bits without recalc
[xxx][xxx][xxx][xxx][xx ][xxx][xxx] 20 bits with current recalc
[xx ][xx ][xx ][xx ][xx ][xx ][xx ] 14 bits with proposed recalc

iomux sections are divided into 32 bit registers with 16 bits for values and 16 bits for writing, without the proposed recalc, there are just too many bits

View: https://bugzilla.kernel.org/show_bug.cgi?id=217334#c4
You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: [PATCH] pinctrl: rockchip: rk3328: rk3328_mux_recalced_data: fix bit alignment

[Kernel.org Bugbot]

antwain.schneider added an attachment on Kernel.org Bugzilla:

Created attachment 304214
both parts of the recalc in one handy patch

david wu's original patch lovingly restored for modern audiences

File: pinctrl-rockchip.diff (text/plain)
Size: 1.61 KiB
Link: https://bugzilla.kernel.org/attachment.cgi?id=304214
---
both parts of the recalc in one handy patch

You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: [PATCH] pinctrl: rockchip: rk3328: rk3328_mux_recalced_data: fix bit alignment

[Kernel.org Bugbot]

antwain.schneider writes via Kernel.org Bugzilla:

*** PROOF OF CONCEPT ALERT ***

here is a fun and easy way to prove how broken the current calculation is using both of the bug reports i have made combined together

using a unmodified kernel with an unmodified dtb, take note of the grf iomux gpio2b registers, it should look like this

00000200 00000001

then modifiy the device tree, change the mode of the otp-out pin from 1 to 2 and then reboot and check the registers again, they will look like this

00000200 00000002

now if you've been playing along, you will be aware that the second 32 bit chunk represents the high-side of gpio2b, which is only pin 7, but otp-out is pin 5, which resides in the first 32 bit chunk which represents the low-side of gpio2b

you can try this on any rk3328 based device

*** PROOF OF CONCEPT ALERT ***

View: https://bugzilla.kernel.org/show_bug.cgi?id=217334#c6
You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: https://bugzilla.kernel.org/show_bug.cgi?id=217334 arm arm64 aarch64 pinctrl rockchip rk3328 rk3328_mux_recalced_data bits overflowing

[Kernel.org Bugbot]

antwain.schneider writes via Kernel.org Bugzilla:

in hopes of making the problem clear, i will attempt to make this as understandable as possible

firstly, i was using a tool to read memory locations without making it known what it was

ma, or memaccess is available on github katsuster/memaccess

also i was using bcm2835-gpiomem from raspberrypi with an alteration of the device tree and udev to allow unprivileged access to poke memory values without root

github raspberrypi/linux drivers/char/broadcom/bcm2835-gpiomem.c
github RPi-Distro/raspberrypi-sys-mods etc.armhf/udev/rules.d/99-com.rules

--- ./orig/rk3328.dtsi
+++ ./rk3328.dtsi
@@ -281,7 +281,7 @@
	};

	grf: syscon@ff100000 {
-		compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+		compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd", "brcm,bcm2835-gpiomem";
		reg = <0x0 0xff100000 0x0 0x1000>;

		io_domains: io-domains {

or alternatively put

fdtput -t s rk3328-rock64.dtb /syscon@ff100000 compatible "rockchip,rk3328-grf" "syscon" "simple-mfd" "brcm,bcm2835-gpiomem"

but in order to make this more accessible, i realized i should use something more generic and familiar that requires no modification, but requires root access

openwrt packages utils/io/src/io.c

for the sake of transparency, i will with great granularity go over everything again with both of these methods and instead of showing only GRF_GPIO2B{L,H}_IOMUX together, will also show them in their individual 32 bit sections

so currently, with a mainline derived kernel with unmodified pinctrl-rockchip, by default you will see these values

% ma -k /dev/gpiomem dd 0x24 8
00000020  -------- 00000200  00000001

% ma -k /dev/gpiomem dd 0x24 4
00000020  -------- 00000200
% ma -k /dev/gpiomem dd 0x28 4
00000020  -------- --------  00000001

# ./io -4 0xff100024
ff100024:  00000200
# ./io -4 0xff100028
ff100028:  00000001

# ./io -l 4 0xff100024
ff100024:  00 02 00 00
# ./io -l 4 0xff100028
ff100028:  01 00 00 00

section 3.3.2 of the trm defines the reset value for GRF_GPIO2BL_IOMUX (0x24/0xff100024) to be 0x0200, and looking in section 3.3.3 at the layout of the mux, in binary with assignments:

resresres2b4reserved---
00 00 00 10 00 00 00 00

gpio2b4 is explicitly defined, with an alignment of 2 bits, while every other bitfield is reserved, but elsewhere in the trm, other pins in gpio2b are defined as existing
in the trm, section 20.5 defines 2b0, 2b1, 2b2, 2b3, 2b4 (again), and section 19.5 defines 2b5 and 2b6, all making clear they have 2 bit alignment, assuming these are true, this would make the bitfield layout look like this:

res2b62b52b42b32b22b12b0
00 00 00 10 00 00 00 00

all of these fit cleanly into the first 16 bits of a mux register, where pin modes are defined

so it's odd that in rk3328.dtsi, gpio2b5 is in use and set to mode 1, but it isn't reflected in the bitfield, so let us change the value and see what happens

--- ./orig/rk3328.dtsi
+++ ./rk3328.dtsi
@@ -1274,3 +1274,3 @@
			otp_out: otp-out {
-				rockchip,pins = <2 RK_PB5 1 &pcfg_pull_none>;
+				rockchip,pins = <2 RK_PB5 2 &pcfg_pull_none>;
			};

and reboot and check values

% ma -k /dev/gpiomem dd 0x24 8
00000020  -------- 00000200  00000002

% ma -k /dev/gpiomem dd 0x24 4
00000020  -------- 00000200
% ma -k /dev/gpiomem dd 0x28 4
00000020  -------- --------  00000002

# ./io -4 0xff100024
ff100024:  00000200
# ./io -4 0xff100028
ff100028:  00000002

# ./io -l 4 0xff100024
ff100024:  00 02 00 00
# ./io -l 4 0xff100028
ff100028:  02 00 00 00

the fact that changing the iomux value of gpio2b5, which if section 19.5 in the trm is to be believed, part of GRF_GPIO2BL_IOMUX, is clearly modifying the value in GRF_GPIO2BH_IOMUX, of which according to chapter 3, defines only one iomux pin, gpio2b7, beginning at offset 0, clearly indicates that bits are being carried in this current calculation and is wrong

the reason for this is that in pinctrl-rockchip.c, rk3328_pin_banks sets gpio2b as 3 bits per pin (which gpio2b7 is), and while rk3328_mux_recalced_data takes care to realign gpio2b4 as stated in chapter 3, it continues to assume every other pin marked as reserved is 3 bits

the patch in this bug report addresses this, adapting the original submission into the current format for recalculation
if anyone has any questions about this, feel free to ask them here
and also i don't want to keep recompiling the kernel, so please commit this!

View: https://bugzilla.kernel.org/show_bug.cgi?id=217334#c7
You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: rk3328_mux_recalced_data OVERFLOW noncontiguous CARRIED bits

[Kernel.org Bugbot]

antwain.schneider writes via Kernel.org Bugzilla:

for the uninitiated who want to join in, the trm being referenced is here[1]
and since this problem is directly related to the rk3328, other vendor kernels have adopted the related changes, for example the firefly roc-cc kernel[2]
i can't imagine why this bug is allowed to remain open leaving many people unable to experience the joy of completely functional gpio2 b4 and gpio2 b7, unless you think the pins work fine as-is and commiters are randomly applying code to forks

[1] https://opensource.rock-chips.com/images/9/97/Rockchip_RK3328TRM_V1.1-Part1-20170321.pdf
[2] https://gitlab.com/firefly-linux/kernel/-/commit/d69af8ab6534bb28c1556076f08d2a5ab4935d95

View: https://bugzilla.kernel.org/show_bug.cgi?id=217334#c8
You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: linux rockchip gpio2b4 (gpio2b5) gpio2b7 broken GRF_GPIO2BL_IOMUX GRF_GPIO2BH_IOMUX 2bits 3bits pinctrl rk3328_mux_recalced_data (a fix exists, for a commiter brave enough to commit it)

[Kernel.org Bugbot]

antwain.schneider writes via Kernel.org Bugzilla:

for more than 6 years pinctrl had been mishandling hardware preventing usage of gpio pins

View: https://bugzilla.kernel.org/show_bug.cgi?id=217334#c9
You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: linux rockchip gpio2b4 (gpio2b5) gpio2b7 broken GRF_GPIO2BL_IOMUX GRF_GPIO2BH_IOMUX 2bits 3bits pinctrl rk3328_mux_recalced_data (a fix exists, for a commiter brave enough to commit it)

[Kernel.org Bugbot]

antwain.schneider writes via Kernel.org Bugzilla:

ping

View: https://bugzilla.kernel.org/show_bug.cgi?id=217334#c10
You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: linux rockchip gpio2b4 (gpio2b5) gpio2b7 broken GRF_GPIO2BL_IOMUX GRF_GPIO2BH_IOMUX 2bits 3bits pinctrl rk3328_mux_recalced_data (a fix exists, for a commiter brave enough to commit it)

[Kernel.org Bugbot]

antwain.schneider added an attachment on Kernel.org Bugzilla:

Created attachment 305795
a program written in c that can be compiled and ran on a rk3328-based device to illustrate the problem which is bad

this program dumps the bits set in the grf for 2bl and 2bh

in comment 7, the otp-out thing is still vital to prove this point without extensive dtb modification

i've got it down to this
fdtput -t x /boot/efi/rockchip/rk3328-roc-cc.dtb /pinctrl/tsadc/otp-out rockchip,pins 02 0d 02 5a
what's being changed in this line is the mode (from 1 to 2) of gpio2b5

otp-out in the rk3328.dtsi is
                                rockchip,pins = <2 RK_PB5 1 &pcfg_pull_none>;
which when dumped by dtc is
                                rockchip,pins = <0x02 0x0d 0x01 0x5a>;
0x5a is the phandle for pcfg_pull_none for me currently, your value may vary
notice that it's using gpio2b5 which exists but doesn't have a listing in chapter 3 of the trm but is described elsewhere in the document


1 run the program and save output
2 fdtput the dtb and reboot
3 run the program again and compare outputs, and notice how the completely wrong register changed

File: poc.c (text/x-csrc)
Size: 1.53 KiB
Link: https://bugzilla.kernel.org/attachment.cgi?id=305795
---
a program written in c that can be compiled and ran on a rk3328-based device to illustrate the problem which is bad

You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)

Re: inux ockchip pio2b4 (pio2b5) pio2b7 roken GRF_PIO2BL_IOMUX GRF_PIO2BH_IOMUX 2bits 3bits pinctrl rk332_mux_ecalced_data

[Kernel.org Bugbot]

antwain.schneider added an attachment on Kernel.org Bugzilla:

Created attachment 305801
here is the page in the trm that describes the registers in question

if you can't read the pdf here is a png

File: 1.png (image/png)
Size: 4.24 MiB
Link: https://bugzilla.kernel.org/attachment.cgi?id=305801
---
here is the page in the trm that describes the registers in question

You can reply to this message to join the discussion.
-- 
Deet-doot-dot, I am a bot.
Kernel.org Bugzilla (peebz 0.1)