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

[Thierry Reding <thierry.reding@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6006 bytes --]

On Tue, Sep 29, 2026 at 03:15:10PM +0530, Kartik Rajput wrote:
> 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.
> 
> Co-developed-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Kartik Rajput <kkartik@nvidia.com>
> ---
>  drivers/ufs/host/Kconfig     |  14 +
>  drivers/ufs/host/Makefile    |   1 +
>  drivers/ufs/host/ufs-tegra.c | 686 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 701 insertions(+)
> 
> diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
> index ff170c0b6da0..7ca317d25251 100644
> --- a/drivers/ufs/host/Kconfig
> +++ b/drivers/ufs/host/Kconfig
> @@ -168,3 +168,17 @@ config SCSI_UFS_AMD_VERSAL2
>  
>  	  Select this if you have UFS controller on AMD Versal Gen 2 SoC.
>  	  If unsure, say N.
> +
> +config SCSI_UFS_TEGRA
> +	tristate "NVIDIA Tegra264 UFS controller platform driver"
> +	depends on SCSI_UFSHCD_PLATFORM
> +	depends on ARCH_TEGRA_264_SOC || (COMPILE_TEST && 64BIT)

From a quick look I couldn't spot anything specific to 64-bit in the
driver. Did I miss anything?

Also, we really shouldn't depend on ARCH_TEGRA_264_SOC since this
controller exists on prior generations and we will eventually want to
support them, too.

> +	select PHY_TEGRA_MPHY

Maybe to complement what Krzysztof already said: Kconfig dependencies
are primarily build-time dependencies. This driver uses the generic PHY
API to access the M-PHY functionality, so GENERIC_PHY is the correct
build-time dependency. The driver is purposefully agnostic to the
specific implementation of the PHYs that it uses so that it can work
with (potentially) many different PHY implementations.

We use device tree to hook up the runtime dependencies. If the M-PHY
driver is not enabled we get the probe deferred because the PHYs that
were hooked up in DT haven't been registered and hence can't be found
(by the generic PHY framework). On other devices the PHYs might end up
being backed by completely different implementations and they could
still work just fine.

> +	help
> +	  Enable support for the UFS host controller on NVIDIA Tegra264
> +	  SoCs. The driver relies on the Tegra264 M-PHY driver

I would suggest dropping references to Tegra264 here. The driver is not
specific to Tegra264. There are older Tegra SoCs that have (earlier)
generations of this IP and that should still work with this driver
(maybe with some parameterization).

> diff --git a/drivers/ufs/host/ufs-tegra.c b/drivers/ufs/host/ufs-tegra.c
> new file mode 100644
> index 000000000000..f12265432592
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-tegra.c
> @@ -0,0 +1,686 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +// NVIDIA Tegra264 UFS host controller driver.

Tegra264 can be dropped here as well.

[...]
> +#define UFS_TEGRA_HS_CLK_RATE_HZ		5840000000UL

Do we assume that this will always be the same value? Maybe this should
be turned into SoC data? We could always do that when necessary, of
course.

[...]
> +static int ufs_tegra_mphy_power_on(struct ufs_hba *hba)
> +{
> +	struct ufs_tegra *ufs = ufshcd_get_variant(hba);
> +	int err;
> +
> +	err = phy_power_on(ufs->mphy_l0_rx);
> +	if (err) {
> +		dev_err(hba->dev, "failed to power on mphy-l0-rx\n");
> +		return err;
> +	}
> +
> +	err = phy_power_on(ufs->mphy_l0_tx);
> +	if (err) {
> +		dev_err(hba->dev, "failed to power on mphy-l0-tx\n");
> +		goto out_power_off_l0_rx;
> +	}
> +
> +	err = phy_power_on(ufs->mphy_l1_rx);
> +	if (err) {
> +		dev_err(hba->dev, "failed to power on mphy-l1-rx\n");
> +		goto out_power_off_l0_tx;
> +	}
> +
> +	err = phy_power_on(ufs->mphy_l1_tx);
> +	if (err) {
> +		dev_err(hba->dev, "failed to power on mphy-l1-tx\n");
> +		goto out_power_off_l1_rx;
> +	}
> +
> +	return 0;
> +
> +out_power_off_l1_rx:
> +	phy_power_off(ufs->mphy_l1_rx);
> +out_power_off_l0_tx:
> +	phy_power_off(ufs->mphy_l0_tx);
> +out_power_off_l0_rx:
> +	phy_power_off(ufs->mphy_l0_rx);
> +
> +	return err;
> +}

Almost seems like we need phy_bulk APIs. Again, doesn't need to be part
of this series, but maybe something to keep in mind.

[...]
> +static int ufs_tegra_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
> +			     enum ufs_notify_change_status status)
> +{
> +	if (status == PRE_CHANGE)
> +		return 0;
> +
> +	/* Runtime H8 park keeps the M-PHY powered; only tear it down
> +	 * on the full-teardown paths (system suspend, shutdown, or
> +	 * runtime PM with LINK_OFF).
> +	 */

Comment style needs the first line to be /* on its own, except for the
net subsystem, if I recall correctly. There are other occurrences of
this elsewhere in the file.

Also, I read "H8" as "hate" at first and it took me a little bit to
realize it meant "hibernate" in this case. Looks like the rest of UFS
uses "hibern8". Maybe stick to the convention?

[...]
> +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->hs_clk, UFS_TEGRA_HS_CLK_RATE_HZ);

What if hs_rate != PA_HS_MODE_A or PA_HS_MODE_B?

> +static int ufs_tegra_set_dma_mask(struct ufs_hba *hba)
> +{
> +	return dma_set_mask_and_coherent(hba->dev, DMA_BIT_MASK(32));
> +}

I have a feeling like the 32 is here on purpose, but I can't fully
remember why it's not something like 40, which is the standard on recent
Tegra devices.

Maybe we should add a comment explaining why this needs to be so narrow.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]