Repository navigation
novatek: pad-mux, reginfo and GPIO for NA51089 (#237) - #239
Merged
Merged
Conversation
PR Summary by QodoAdd NA51089 pad mux, register dumps, and GPIO support
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Code Review by Qodo
1. MIPI pads can report the wrong function
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| {161, 0, PMX_SENSOR_CCIR_2, 171, 2}, /* H_GPIO1 */ | ||
| {161, NVT_UNGATED, PMX_SENSOR_12BITS, 137, 1}, /* H_GPIO1 */ | ||
| {161, 0, PMX_SENSOR_CCIR_2, 138, 1}, /* H_GPIO1 */ | ||
| {161, 0, PMX_MIPI_LVDS_DAT0, 1, 0}, /* H_GPIO1 */ |
There was a problem hiding this comment.
1. Mipi pads can report the wrong function 🐞 Bug ≡ Correctness
The generator builds MIPI lane claims from the target call's gate writes but omits the prerequisite sensor-mode writes, leaving those claims with no conditions. When a lane's gate is clear outside CSI mode, get() can report it as MIPI, while set() can choose the gate-only claim without selecting CSI mode.
Agent Prompt
## Issue description
MIPI lane claims have no sensor-mode condition because prefix writes are omitted when the generator constructs claims.
## Fix Focus Areas
- tools/gen_novatek_padmux.py[540-579]
- tools/gen_novatek_padmux.py[741-752]
- src/hal/novatek_padmux.h[636-640]
## Recommended Fix
Preserve the prerequisite sensor-mode state when generating dependent claims, regenerate the table, and test reads and writes in both CSI and non-CSI modes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+191
to
+195
| .gpio_func = (int)bit, | ||
| /* Setting the gate is the whole operation -- unless a sensor mode | ||
| * can hold the pad through it, and then it is not. */ | ||
| .flags = IPCHW_PADMUX_F_GPIO | | ||
| (has_ungated(soc, pd->pad) ? 0 : IPCHW_PADMUX_F_RMW), |
There was a problem hiding this comment.
3. Gpio restoration can leave sensor pads busy 🐞 Bug ≡ Correctness
gpio_row() advertises the gate bit as a one-write GPIO restoration even when the pad has an ungated sensor claim. If that claim holds, setting the gate alone leaves the sensor function active; nvt_set() must also drop the competing claim.
Agent Prompt
## Issue description
GPIO rows expose a one-write restoration value for pads whose sensor claims can remain active through the gate.
## Fix Focus Areas
- src/hal/novatek_padmux.c[180-196]
- src/hal/novatek_padmux.c[369-374]
## Recommended Fix
Set gpio_func to -1 when the pad has an ungated claim, so callers use ipchw_padmux_set() for the complete restoration.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .func_name = padmux_name(s->name), | ||
| .gpio_name = padmux_name(pd->name), | ||
| .gpio_pad = pd->pad, | ||
| .gpio_func = -1, |
There was a problem hiding this comment.
4. Gate-only pads lose their restore hint 🐞 Bug ≡ Correctness
func_row() sets gpio_func to -1 for every peripheral row, including gate-only functions on pads without ungated claims. When get() returns such a function, its row omits the valid single-write instruction to restore GPIO by setting the gate bit.
Agent Prompt
## Issue description
Gate-only functions return no GPIO restoration hint despite having a single-write restoration on pads without ungated claims.
## Fix Focus Areas
- src/hal/novatek_padmux.c[199-220]
- src/hal/novatek_padmux.c[282-287]
## Recommended Fix
For a winning gate-only claim where setting the gate suffices, populate gpio_func with the pad's gate bit; retain -1 when another claim requires additional writes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+398
to
+408
| for (int i = 0; i < best->ncond; i++) { | ||
| const novatek_cond_t *k = &soc->conds[best->cond + i]; | ||
| if (!top_write_field(&t, k->reg, field_mask(k->shift, k->width), | ||
| (uint32_t)k->value << k->shift)) | ||
| return IPCHW_PADMUX_IO; | ||
| } | ||
| if (!(best->flags & NVT_UNGATED) && | ||
| !top_write_field(&t, gr, gate_bit(pad), 0)) | ||
| return IPCHW_PADMUX_IO; | ||
|
|
||
| return drop_competitors(soc, pad, &t, best); |
There was a problem hiding this comment.
5. A refused pad-mux change still rewrites registers 🐞 Bug ☼ Reliability
nvt_set() writes every field of the chosen claim and clears the pad's gate bit before it calls drop_competitors(). That helper can still return IPCHW_PADMUX_NO_FUNC when no claim wins, when every non-zero field of the winning competitor is one the target needs, or when its 8 passes run out, and nothing rolls back the earlier writes. When that happens, gpio mux <pad> <func> prints "cannot carry", yet the board has already moved a peripheral's location field (which unmuxes it from its other pads) and handed the pad off the GPIO controller.
Agent Prompt
## Issue description
`nvt_set()` writes the target claim's fields and clears the gate, then calls `drop_competitors()`, which can fail with IPCHW_PADMUX_NO_FUNC. The writes already done are never undone, so a refused set still changes the hardware.
## Fix Focus Areas
- src/hal/novatek_padmux.c[357-409]
- src/hal/novatek_padmux.c[315-341]
## Recommended Fix
Record the original value of every TOP word that `nvt_set` writes. The `top_t` cache already holds the first-read values, so copy `t.val`/`t.have` before writing. If `drop_competitors` (or any later write) fails, write the saved words back before returning the error. Another option is to simulate the sequence first on a scratch `top_t` whose io writes only update the cache, and touch the real registers only when the simulation succeeds.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+436
to
+437
| if (strchr(spec, '_')) | ||
| return -1; |
There was a problem hiding this comment.
6. Gpio usage text offers a pad format novatek refuses 🐞 Bug ⚙ Maintainability
gpio_get_cmd/gpio_set_cmd/gpio_mux_cmd still print "<gpio number> either number in 5_6 or 46 format", but on NA51089 nvt_parse_pad() deliberately returns -1 for any spelling containing '_' that is not a known pad-group prefix. A Novatek user who follows the usage hint gets "Not a GPIO number", and the kernel names this PR adds (P_GPIO22, MC17) are not mentioned anywhere in the CLI help.
Agent Prompt
## Issue description
The gpio subcommand usage says pads can be given as 5_6 or 46, but on Novatek the 5_6 form is refused and kernel names such as P_GPIO22 are accepted.
## Fix Focus Areas
- src/reginfo.c[3740-3756]
- src/reginfo.c[3891-3899]
## Recommended Fix
Change the usage text to say that 5_6 is the HiSilicon form, and that Novatek takes the Linux GPIO number or the kernel's pad name (P_GPIO22, MC17, HSI_GPIO9).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
`ipctool reginfo` on an NT98566 said "Platform is not supported": the Novatek HAL never set chip_generation, and no Novatek table existed. The NA51089 selects a pad's function a fourth way: a location field per peripheral in the TOP block (SigmaStar's arrangement), plus a gate bit per pad that has to be cleared before any field reaches it. A new backend, src/hal/novatek_padmux.c, reads that as claims -- a name and the field values under which the pad carries it -- and takes the first that holds. set() writes the claim that changes least, clears the gate, and drops a competitor that still wins by zeroing a field the target does not need. There is no datasheet or pin table in the SDK, only the vendor driver's pinmux_config_*() functions. tools/gen_novatek_padmux.py compiles those on the host with every register write logged, runs every combination of up to four options (96,550 calls), attributes each gate write to the field writes of the blocks around it, and replays every state against the table. Names come from top.h's annotations and top.csv; three places where those disagree with the code are recorded as errata, one of them a vendor bug that makes PIN_MISC_CFG_SP2CLK_3RD do nothing. Also: - chip_generation is the TOP chip-ID word. - plain `reginfo` dumps the TOP mux registers and the eight gate bitmaps. - `gpio get/set/scan` and the IR-cut hint drive the NA51089 GPIO controller (DATA/DIR/SET/CLR at 0xF0070000). - pads may be given as the Linux GPIO number or the kernel's name (P_GPIO22, MC17, HSI_GPIO9); the HiSilicon 5_2 form is refused there. No Novatek board is in the lab: verified against vendor source, by reginfo_test's sweep and test_novatek(), and by the generator's replay. ipcinfo does not grow; ipctool grows 16 KB on arm32 (9 KB after UPX).
widgetii
force-pushed
the
novatek-padmux-237
branch
from
October 9, 2026 19:54
dbbf3a2 to
3f1662b
Compare
…lds the pad From review of #239: - set() that refuses after writing a claim's fields now writes back every TOP word it touched, so a refusal never leaves a peripheral moved half-way. - the GPIO row of a pad an ungated sensor claim can hold carries gpio_func -1, since setting the gate alone does not free it. - gpio usage text names the pad spellings each vendor takes. - reginfo_test sets every function of a pad from every claim's own state (2110 sets) and checks it reads back, or that a refusal changed nothing.
Member
Author
|
Review findings, one by one:
|
This was referenced Oct 9, 2026
widgetii
added a commit
that referenced
this pull request
Oct 10, 2026
…me (#240) * novatek: NA51055 and NA51084 tables, the primary LCD, and 0x5021's name The three things #239 left open. NA51055 (NT9852x) and NA51084 (NT98528/NT98529). The NA51089 SDK carries the na51055 driver too: same TOP layout without the DSI group, more S, L and D pads, and one driver for both dies that branches on nvt_get_chip_id(). The generator now runs a driver per family and each family once per die, with the chip-ID call answered by a constant, and writes one table per die. NA51084 alone has I2C4/5, SPI4/5 and RGMII. Their top.h annotations disagree with their code in ten places -- MISC copied from NA51089, PWM8..11's HSI location commented out ("52x compatible") -- so the errata are now per driver; NA51089's PICNT2 erratum is exactly wrong on NA51055. The LCD. pinmux_select_primary_lcd() is run as every LCD type with every set of feature flags, its local copies of REG2 and the L and DSI gates turned into the globals every other function writes. That needed one rule more: a field an enclosing block wrote and a nested block overwrote before the gate write takes the nested value (LCD_TYPE is reset at the top and set in a switch). And options are read only from `& PIN_...` tests and case labels, so a range test cannot name a pad. TV and HDMI have no pads on these dies: pinmux_set_host() answers E_ID for them. Two resolver fixes the new tables exposed. Of two claims with alike conditions the gated one now wins, and set() of an ungated claim writes the gate to GPIO as the vendor does: CCIR8 data and CCIR8 sync both hold under SENSOR2=2. The replay now reports a refinement that changes nothing instead of passing over it silently. 0x5021 was named NT98562. It is NA51084: u-boot's NA51084 clock code drives the _528_ registers and allows 1200 MHz only on an NT98529. It now reports NT98528. NT98562 and NT98566 share 0x7021 and are told apart only by an eFuse word the SDK decodes in a binary. Not done, and why: NA51090 and NA51103 have no pinmux driver in this SDK or anywhere public that was found (NA51090's registers are also above 4 GB); NA51068 is a per-pad selector part with no chip ID here. reginfo_test sweeps all three dies and sets every function from every claim's state (3201, 2118 and 2342 sets, none refused). ipcinfo is byte-identical on arm32; ipctool grows 20 KB (6.5 KB after UPX). * novatek: LCD feature flags were never run; names stay within a group From review of #240. The LCD flags (TE, DE, HVLD/VVLD, FIELD, NO_HVSYNC) are spelt `0x01 << 23` in top.h, which the enum parser read as no value, so they were taken for LCD types: the harness never ran a type with DE, every colour claim required DE off, and LCD_DE_ENABLE required LCD_TYPE = GPIO. A real panel with DE read as unclaimed and set(LCD_DE_ENABLE) would have turned it off. Enumerator values are now evaluated for shifts and ors, and an LCD enum with no readable flags is fatal. That exposed a second bug: a state no source names inherited the name of any annotated state whose conditions are a subset of its own, across groups. NA51055's RMII claims L_GPIO0..10 with no condition at all, so every LCD claim there was named TXD0, CRS_DV and so on. Inheritance is now within one group. Also: - bonded_pads() skips a missing count macro only for the DSI group; any other missing one is a renamed macro and is fatal again. - NA51055 is named by its die when the device tree has no model, so a board without one is still detected. - novatek_open_i2c_fd() tests NA51103 by chip ID: the ID path names it NT98332G, so the "NA51103" name test never matched and its sensor bus was never 1. - nvt_get_chip_id() names through one strcpy; ipcinfo stays byte-identical. reginfo_test: a DE panel reads as the LCD on both the colour and the DE pin, and setting DE keeps the panel type. Every-state sets: 4363, 4038 and 4347, none refused. The selftest covers shifted enum values and cross-group inheritance, and fails with either fix reverted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #237.
On an NT98566,
ipctool reginfoprinted "Platform is not supported". The Novatek HAL never setchip_generation, and there was no Novatek table.What the NA51089 does
The NA51089 selects a pad's function in a fourth way, different from the existing vendors:
0xF0010000, as on SigmaStar. For example,REG5[7:6]is I2C3: 1, 2 and 3 put it onP_GPIO21/22,C_GPIO11/12orDSI_GPIO8/9.TOP+0xA0..0xE8. Setting it keeps the pad a GPIO, whatever the fields say.The new backend,
src/hal/novatek_padmux.c, reads this as claims: a name, plus the field values under which the pad carries it. The first claim that holds is the answer.set():Moving a peripheral to another location leaves its old pads cleared with nothing claiming them.
get()reports such a pad as unnamed, which is what the hardware state is.Where the table comes from
The SDK has no datasheet and no pin table, only the driver's
pinmux_config_*()functions.tools/gen_novatek_padmux.py:Names come from
top.h's@PAD[NAME]annotations, then fromtop.csv. Three places where those disagree with the code are listed inANNOTATION_ERRATA, each with the reason:P_GPIO29for SIF CH2_2P_GPIO19P_GPIO14for PICNT2_1L_GPIO1PIN_MISC_CFG_SP2CLK_3RDpinmux_config_misc()tests the SENSOR constant there, so the option does nothing--selftestruns against a built-in fixture SDK and needs only a C compiler.tools/test_pipeline.shpicks it up automatically.Also in this PR
chip_generationis now the TOP chip-ID word.getchipfamily()is unchanged: it still returns the chip name, soipcinfostays byte-identical.reginfodumps the TOP mux registers and the eight gate bitmaps.gpio get/set/scanand the IR-cut hint drive the GPIO controller at0xF0070000(DATA, DIR, SET, CLR).P_GPIO22,MC17,HSI_GPIO9). The HiSilicon5_2form is refused on Novatek, because it would land on a different pad.IPCHW_PADMUXfamily token:novatek.Verification
reginfo_test:test_novatek(), whose register values are taken from host.c;set()makes it fail.tools/test_pipeline.shpasses, and so dogen_padmux_names.py --checkandgen_novatek_padmux.py --verifyagainst the SDK.IPCHW_PADMUX=none,IPCHW_VENDORS=ingenic,IPCHW_PADMUX=novatek;IPCHW_VENDORS=novatekandIPCHW_PADMUX=sstar.ipcinfois byte-identical to master, withalland withnone. A first version added agetchipfamily()case, 64 bytes, which pushed the RW segment onto the next page and cost a full 4 KB, so it was dropped;ipctoolgrows by 16 KB, or 9 KB after UPX.Not covered
ipctool reginfo --padswithcat /proc/nvt_info/nvt_pinmux/pinmux_summary, the kernel's own decode of the same registers.0x5021to "NT98562" looks wrong: the SDK's top.h calls0x5021NA51084, while NT98562 and NT98566 are both NA51089 (0x7021) and differ only in an eFuse package ID. This PR leaves that alone.