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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ufs: tegra: missing hardware reset assertion in teardown path
- [Medium] ufs: tegra: missing dummy readl() after writel() to guarantee delay timing and ordering
--

commit 1b8e042f5d752753eca62f28d9beaa880236dcb0
Author: Kartik Rajput <kkartik@nvidia.com>

scsi: ufs: tegra: Add Tegra264 UFS host controller driver

Add a driver for the UFS host controller found on NVIDIA Tegra264 SoCs.
The controller has Tegra-specific auxiliary registers, clocks and resets,
and it drives the four M-PHY lane directions exposed by the Tegra264
M-PHY driver.

> diff --git a/drivers/ufs/host/ufs-tegra.c b/drivers/ufs/host/ufs-tegra.c
> new file mode 100644
> index 0000000000000..f122654325924
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-tegra.c
[ ... ]
> +static void ufs_tegra_configure_aux(struct ufs_hba *hba)
> +{
> +	struct ufs_tegra *ufs = ufshcd_get_variant(hba);
> +	u32 value;
> +
> +	value = readl(ufs->aux + UFSHC_AUX_DEV_CTRL);
> +	value |= DEV_CTRL_CLK_EN;
> +	writel(value, ufs->aux + UFSHC_AUX_DEV_CTRL);
> +
> +	/* Let the reference clock stabilise before deasserting device reset. */
> +	usleep_range(1000, 2000);

[Severity: Medium]
Memory-mapped peripheral writes via writel() provide CPU ordering but do not
guarantee that the write has reached the interconnect. Does this sleep begin
without guaranteeing the clock enable write has reached the peripheral?

Without a dummy readl() to flush the posted write, the CPU can proceed while
the write is still buffered, causing the timing delay to execute prematurely.

> +
> +	value |= DEV_CTRL_RESET;
> +	writel(value, ufs->aux + UFSHC_AUX_DEV_CTRL);
> +}
> +
> +static void ufs_tegra_aux_init(struct ufs_hba *hba)
> +{
> +	struct ufs_tegra *ufs = ufshcd_get_variant(hba);
> +	u32 value;
> +
> +	value = readl(ufs->aux + UFSHC_AUX_DEV_CTRL);
> +	value &= ~(DEV_CTRL_CLK_EN | DEV_CTRL_RESET);
> +	writel(value, ufs->aux + UFSHC_AUX_DEV_CTRL);
> +
> +	/* Ensure the clear is observed before we re-enable. */
> +	usleep_range(100, 200);

[Severity: Medium]
Similarly here, does this sleep begin without guaranteeing the clear operation
has reached the peripheral? A dummy readl() might be needed to flush the
posted write before delaying.

> +
> +	ufs_tegra_configure_aux(hba);
> +}
[ ... ]
> +static int ufs_tegra_hce_enable_notify(struct ufs_hba *hba, enum ufs_notify_change_status status)
> +{
> +	struct ufs_tegra *ufs = ufshcd_get_variant(hba);
> +	u32 value;
> +	int err;
> +
> +	switch (status) {
> +	case PRE_CHANGE:
> +		err = ufs_tegra_power_up_sequence(hba);
> +		if (err)
> +			return err;
> +
> +		/* Assert device reset; POST_CHANGE releases it via
> +		 * ufs_tegra_configure_aux() so the HCE toggle pulses it.
> +		 */
> +		value = readl(ufs->aux + UFSHC_AUX_DEV_CTRL);
> +		value &= ~DEV_CTRL_RESET;
> +		writel(value, ufs->aux + UFSHC_AUX_DEV_CTRL);
> +		break;

[Severity: Medium]
Does this write need to be flushed before returning? Without flushing the
auxiliary write, the core driver proceeds to write REG_CONTROLLER_ENABLE.
The HCE toggle might beat the reset assertion, causing hardware initialization
failures.

> +	case POST_CHANGE:
[ ... ]
> +static void ufs_tegra_exit(struct ufs_hba *hba)
> +{
> +	ufs_tegra_mphy_disable(hba);
> +}

[Severity: Medium]
Does this teardown function lack a call to reset_control_bulk_assert()?

The hardware resets are initially deasserted in ufs_tegra_init() via
reset_control_bulk_deassert(). However, the teardown callback
ufs_tegra_exit() misses the matching assertion. The API
devm_reset_control_bulk_get_exclusive() only releases the references but does
not auto-assert the reset lines upon unbind.

This leaves the UFS host controller hardware unquiesced (active) when the
driver is unbound, potentially leading to state issues on kexec or power
leaks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-tegra264-ufs-v2-0-f0467b9c503e@nvidia.com?part=4