Re: [PATCH 4/4] scsi: ufs: tegra: Add Tegra264 UFS host controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Referencing a non-existent Kconfig symbol `PHY_TEGRA_MPHY` in `select`.
- [Medium] Unmet dependency risk due to unconditionally selecting a potentially architecture-specific symbol in a driver that supports `COMPILE_TEST`.
- [Medium] Integer truncation of the 5.84 GHz clock rate on 32-bit architectures during `COMPILE_TEST`.
- [Medium] Hardware PHY teardown sequence incorrectly uses forward setup order instead of reverse.
--

commit ba95657952b3e5dc9efe191a253d2fb49a5a1f66
Author: Kartik Rajput <kkartik@nvidia.com>
Subject: scsi: ufs: tegra: Add Tegra264 UFS host controller driver

This commit adds a new UFS host controller driver for NVIDIA Tegra264 SoCs.
It implements Tegra-specific auxiliary registers, clocks, and resets, and
drives four M-PHY lane directions.

> diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
> index ff170c0b6da0c..e4c565fce53cc 100644
> --- a/drivers/ufs/host/Kconfig
> +++ b/drivers/ufs/host/Kconfig
> @@ -168,3 +168,16 @@ config SCSI_UFS_AMD_VERSAL2
[ ... ]
> +config SCSI_UFS_TEGRA
> +	tristate "NVIDIA Tegra264 UFS controller platform driver"
> +	depends on SCSI_UFSHCD_PLATFORM && (ARCH_TEGRA_264_SOC || COMPILE_TEST)
> +	select PHY_TEGRA_MPHY

[Severity: Medium]
Does the PHY_TEGRA_MPHY symbol exist in the kernel tree? If not, selecting it
here will cause silent build failures or leave features disabled.

[Severity: Medium]
Additionally, since this driver supports COMPILE_TEST, does unconditionally
selecting an architecture-specific PHY risk triggering unmet dependency
warnings on unsupported architectures during cross-compilation?

> diff --git a/drivers/ufs/host/ufs-tegra.c b/drivers/ufs/host/ufs-tegra.c
> new file mode 100644
> index 0000000000000..859fe0af66e51
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-tegra.c
[ ... ]
> +#define UFSHC_AUX_DEV_CTRL			0x14
> +#define DEV_CTRL_CLK_EN				BIT(0)
> +#define DEV_CTRL_RESET				BIT(1)
> +
> +#define UFS_TEGRA_PHY_CLK_HS_RATE_HZ		5840000000UL

[Severity: Medium]
Is this literal too large for a 32-bit unsigned long? The literal requires
33 bits, but when COMPILE_TEST is enabled on 32-bit architectures, this
value will be truncated.

[ ... ]
> +static void ufs_tegra_mphy_exit(struct ufs_hba *hba)
> +{
> +	struct ufs_tegra *ufs = ufshcd_get_variant(hba);
> +
> +	phy_exit(ufs->mphy_l0_rx);
> +	phy_exit(ufs->mphy_l0_tx);
> +	phy_exit(ufs->mphy_l1_rx);
> +	phy_exit(ufs->mphy_l1_tx);
> +}

[Severity: Medium]
Does this exit sequence incorrectly mirror the forward initialization order?
Standard resource teardown typically reverses the initialization sequence to
prevent state corruption or lockups upon driver unbind or runtime suspend.

[ ... ]
> +static void ufs_tegra_mphy_power_off(struct ufs_hba *hba)
> +{
> +	struct ufs_tegra *ufs = ufshcd_get_variant(hba);
> +
> +	phy_power_off(ufs->mphy_l0_rx);
> +	phy_power_off(ufs->mphy_l0_tx);
> +	phy_power_off(ufs->mphy_l1_rx);
> +	phy_power_off(ufs->mphy_l1_tx);
> +}

[Severity: Medium]
Similarly, does this power off sequence execute in exact forward
initialization order instead of reverse?

[ ... ]
> +static int ufs_tegra_pwr_change_notify(struct ufs_hba *hba,
> +				       enum ufs_notify_change_status status,
> +				       struct ufs_pa_layer_attr *dev_req_params)
> +{
[ ... ]
> +	if (dev_req_params->hs_rate == PA_HS_MODE_A ||
> +	    dev_req_params->hs_rate == PA_HS_MODE_B) {
> +		err = clk_set_rate(ufs->phy_clk, UFS_TEGRA_PHY_CLK_HS_RATE_HZ);

[Severity: Medium]
The clk_set_rate() API takes an unsigned long for the rate. Does passing
the 33-bit UFS_TEGRA_PHY_CLK_HS_RATE_HZ literal cause an implicit narrowing
conversion and incorrect clock rate configuration on 32-bit systems?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-tegra264-ufs-v1-0-e4a9831702a0@nvidia.com?part=4