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

[Larisa Ileana Grigore <larisa.grigore@oss.nxp.com>]

On 10/24/2026 5:20 PM, Bart Van Assche wrote:
> 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.
> 

ufshcd_vops_link_startup_notify(hba, PRE_CHANGE) is not checked in 
ufshcd_link_startup(), and this applies to every host driver, not only 
S32N79. Because this is core behaviour and not specific to this driver, 
I would prefer not to change it in this series. A fix in the core that 
checks the PRE_CHANGE return value can follow as a separate patch, so 
that it gets reviewed on its own and does not hold up this driver.

> ## 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.
> 

Will fix in v10.

> **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.
Will drop the "Fixes" tag and will add a new commit which resolves the 
actual problem. __ufshcd_wl_resume will no longer return a positive 
value in case of an error.

Best regards,
Larisa