Bug 217334 - RK3328
Summary: RK3328
Status: RESOLVED CODE_FIX
Alias: None
Product: Linux
Classification: Unclassified
Component: Kernel (show other bugs)
Hardware: ARM Linux
: P3 enhancement
Assignee: Virtual assignee for kernel bugs
URL:
Keywords:
Depends on:
Blocks: 217444
  Show dependency tree
 
Reported: 2023-04-14 08:57 UTC by antwain.schneider
Modified: 2024-07-05 04:25 UTC (History)
0 users

See Also:
Kernel Version:
Subsystem: ARM/Rockchip SoC support
Regression: No
Bisected commit-id:
mricon: bugbot+


Attachments
the other half (743 bytes, patch)
2023-04-21 10:28 UTC, antwain.schneider
Details | Diff
correct other half (749 bytes, patch)
2023-04-21 10:40 UTC, antwain.schneider
Details | Diff
both parts of the recalc in one handy patch (1.61 KB, patch)
2023-05-04 11:00 UTC, antwain.schneider
Details | Diff
a program written in c that can be compiled and ran on a rk3328-based device to illustrate the problem which is bad (1.53 KB, text/x-csrc)
2024-01-30 18:50 UTC, antwain.schneider
Details
here is the page in the trm that describes the registers in question (4.24 MB, image/png)
2024-01-31 21:01 UTC, antwain.schneider
Details

Description antwain.schneider 2023-04-14 08:57:43 UTC
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
Comment 1 antwain.schneider 2023-04-19 08:37:30 UTC
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
Comment 2 antwain.schneider 2023-04-21 10:28:13 UTC
Created attachment 304169 [details]
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
Comment 3 antwain.schneider 2023-04-21 10:40:23 UTC
Created attachment 304170 [details]
correct other half

sorry, off by 8 error :O
Comment 4 antwain.schneider 2023-04-22 21:07:22 UTC
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
Comment 5 antwain.schneider 2023-05-04 11:00:02 UTC
Created attachment 304214 [details]
both parts of the recalc in one handy patch

david wu's original patch lovingly restored for modern audiences
Comment 6 antwain.schneider 2023-05-19 07:05:56 UTC
*** 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 ***
Comment 7 antwain.schneider 2023-06-07 11:52:54 UTC
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!
Comment 8 antwain.schneider 2023-06-28 09:18:49 UTC
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
Comment 9 antwain.schneider 2023-08-27 14:50:24 UTC
for more than 6 years pinctrl had been mishandling hardware preventing usage of gpio pins
Comment 10 antwain.schneider 2024-01-29 14:14:38 UTC
ping
Comment 11 antwain.schneider 2024-01-30 18:50:27 UTC
Created attachment 305795 [details]
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
Comment 12 antwain.schneider 2024-01-31 21:01:07 UTC
Created attachment 305801 [details]
here is the page in the trm that describes the registers in question

if you can't read the pdf here is a png
Comment 13 antwain.schneider 2024-07-05 04:25:33 UTC
thank you Huang-Huang Bao for addressing this problem better than i with commits
commit e8448a6c817c2aa6c6af785b1d45678bd5977e8d
commit 5ef6914e0bf578357b4c906ffe6b26e7eedb8ccf
commit 01b4b1d1cec48ef4c26616c2fc4600b2c9fec05a

GRF_GPIO2BL_IOMUX: 00000600
 2b0 | 2b1 | 2b2 | 2b3 | 2b4 | 2b5 | 2b6 | res
00 01|02 03|04 05|06 07|08 09|10 11|12 13|14 15
 0  0  0  0  0  0  0  0  0  1  1  0  0  0  0  0 
GRF_GPIO2BH_IOMUX: 00000000
   2b7  | reserved.............................
00 01 02|03 04 05 06 07 08 09 10 11 12 13 14 15
 0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0

Note You need to log in before you can comment on or make changes to this bug.