Re: [PATCH 00/10] Add S32N79RDB UFS support

[Bart Van Assche]

On 10/24/26 5:25 AM, Larisa Grigore wrote:
> This series adds support for the UFS host controller found on the NXP S32N79
> platform. The controller is based on the Synopsys DesignWare (DWC) UFS host
> architecture and requires S32N79-specific M-PHY initialization/calibration
> sequence. It is compliant with JESD-223D "Universal Flash Storage Host
> Controller Interface" v3.0, and supports up to HS-Gear 4 speed grade. It is
> compatible with UFS 2.1, 3.0 and 3.1 devices. The MIPI UniPro is compliant with
> "MIPI Alliance Specification for Unified Protocol", v1.8. The MIPI M-PHY is
> compliant to version 4.1.

Please take a look at the report below. It comes from an LLM and hence
may include errors:
----------------------------------------------------------------------

## Main finding

**Patch 9/10: PRE_CHANGE errors are ignored.** In 
`ufshcd_link_startup()` the core calls the PRE_CHANGE hook without 
checking what it returns:
```c
do {
	ufshcd_vops_link_startup_notify(hba, PRE_CHANGE);
	ret = ufshcd_dme_link_startup(hba);
```
So if `ufs_s32n7_link_startup_pre_change()` fails, DME_LINKSTARTUP is 
still sent. That covers an invalid `one_us_tick`, an SRAM_INIT_DONE 
timeout after up to 3 s, a calibration failure, or lanes not reaching 
Hibern8. The error codes and the `dev_err()` calls in that path have no 
effect. Either the series checks the PRE_CHANGE return value in the core 
(with an audit of the existing hooks), or the commit message should say 
that these failures only get logged.

## Per-patch comments

**Cover letter / all patches**
- The subject says `[PATCH 00/10]` without `v9`, but the body says 
"Changes in v9". It should be `[PATCH v9 nn/10]`.
- Patches 1, 5, 6, 8 and 9 start with `scsi: ufs:`. Drop the `scsi:` so 
they read, for example, `ufs: dwc: …` and `ufs: s32n7: …`. That matches 
patches 2 and 3 (`ufs: unipro:`).

**Patch 4/10 (Hibern8 TX FSM polling helper)**
- The prefix is `ufs: host:`, but the helper is added to 
`drivers/ufs/core/ufshcd.c`. It should be `ufs: core:`.
- The commit message names `ufshcd_check_hibern8()`, but the function is 
called `ufshcd_dme_check_tx_hibern8()`.
- "Unlike the initial implementation, the timeout error is now reported 
only once…" describes earlier versions. It belongs below the `---` line.
- Please don't use `extern` in new prototypes in `include/ufs/ufshcd.h`.
- The HiSilicon and Qualcomm messages used to print the TX_FSM_STATE 
value(s) when a lane failed to reach Hibern8. The new helper only logs a 
timeout, so that diagnostic information is lost. Consider logging the 
lane and its last state.

**Patch 5/10 (move CREG helpers)**
- "Dropped `static` from …" is not in the imperative mood.
- The commit message should say why removing `static` matters. In 
Versal2, `phy_write_attrs[]`/`phy_read_attrs[]` were static arrays that 
got modified on every call. Two callers at once, for example two host 
instances, could corrupt each other's values. The move fixes that race, 
which is worth stating.

**Patch 9/10 (S32N79 driver)**
- **Hard-coded lane count:** `s32n79_reset_config[]`, 
`pwr_change_phy_regs[]` and the calibration tables only cover lanes 0 
and 1. But `ufshcd_dme_check_tx_hibern8()` is passed 
`hba->lanes_per_direction`. Use a fixed 2 consistently, or check 
`lanes_per_direction` in `init`.
- **Connection setup:** `ufs_s32n7_link_startup_post_change()` repeats a 
subset of `ufshcd_dwc_connection_setup()`: local attributes only, no 
peer attributes and no `N_DEVICEID_VALID`. Please explain why the shared 
DWC helper, or `ufshcd_dwc_link_startup_notify()` with a 
`phy_initialization` hook, can't be used.
- **Unexplained settings:** `UFSHCD_QUIRK_PERFORM_LINK_STARTUP_ONCE` and 
`hba->spm_lvl = UFS_PM_LVL_5` need a reason in the commit message, for 
example whether power is cut during system suspend.
- **Includes:** `FIELD_PREP()` and `GENMASK()` are used without 
`<linux/bitfield.h>` or `<linux/bits.h>`. It compiles only because 
`ufshcd.h` happens to pull them in.
- **Nits:**
  - Use `u32 data` instead of `unsigned int data` for `ufshcd_dme_get()`.
  - `SCM_ONE_US_TICK 0x2CU` has a `U` suffix but `SCM_STATUS 0x30` doesn't.
  - The comment says "SCM_MPHY_RAM_CONFIG_STATUS" but the macro is 
`SCM_STATUS`.
  - The `dev_err()` in `ufs_s32n7_link_startup_notify()` repeats 
messages that the inner functions already print.
  - `device_get_match_data()` is called in both probe and `init`.
  - `ufs_s32n7_remove()` is a wrapper that adds nothing.

**Patch 1/10:** the change is fine. The `Fixes:` tag is debatable: the 
old positive return of 1 was already treated as a failure by `if (ret)` 
callers, so this isn't really a bug fix for stable.

To check these points I read the code in your local tree (`locking` 
branch). I did not apply or build the series.