Re: [PATCH v2 3/3] riscv: dts: spacemit: k3: Add UFS support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Typo in the UFS reference clock frequency in opp-2 (192 MHz instead of 19.2 MHz).
- [High] UFS device is hard-reset upon entering the Hibern8 suspend state.
- [Low] The device tree references an undefined clock macro CLK_APMU_UFS_REFCLK, which will cause a fatal compilation error.
- [Low] Invalid use of bitwise OR to combine negative integer error codes.
- [Low] Missing FSM state dump in workqueue context as claimed by the event handler.
--

--- Patch [3]: [PATCH v2 3/3] riscv: dts: spacemit: k3: Add UFS support ---
commit c3b7015d657d8f4204038196ab74adb7966ad486
Author: Yixun Lan <dlan@kernel.org>

riscv: dts: spacemit: k3: Add UFS support

This commit adds UFS Host Controller support for the SpacemiT K3 SoC. It also
enables the UFS functionality on the Pico-ITX and CoM260-IFX boards.

> diff --git a/arch/riscv/boot/dts/spacemit/k3.dtsi b/arch/riscv/boot/dts/spacemit/k3.dtsi
> --- a/arch/riscv/boot/dts/spacemit/k3.dtsi
> +++ b/arch/riscv/boot/dts/spacemit/k3.dtsi
> @@ -469,6 +469,33 @@ usb2_phy: phy@c0a20000 {
>  			status = "disabled";
>  		};
>  
> +		ufshc: ufshc@c0e00000 {
> +			compatible = "spacemit,k3-ufshc";
> +			reg = <0x0 0xc0e00000 0x0 0x40000>;
> +			clocks = <&syscon_apmu CLK_APMU_UFS_ACLK>,
> +				 <&syscon_apmu CLK_APMU_UFS_REFCLK>;

[Severity: Low]
Does this code reference an undefined clock macro? CLK_APMU_UFS_REFCLK is not
defined in the included headers, which will cause a fatal syntax error during
device tree compilation.

> +			clock-names = "aclk", "ref_clk";
> +			resets = <&syscon_apmu RESET_APMU_UFS_ACLK>;
> +			interrupts = <135 IRQ_TYPE_LEVEL_HIGH>;
> +			lanes-per-direction = <2>;
> +			operating-points-v2 = <&ufs_opp_table>;
> +			status = "disabled";
> +
> +			ufs_opp_table: opp-table {
> +				compatible = "operating-points-v2";
> +
> +				opp-1 {
> +					opp-hz = /bits/ 64 <491520000>,
> +						 /bits/ 64 <19200000>;
> +				};
> +
> +				opp-2 {
> +					opp-hz = /bits/ 64 <491520000>,
> +						 /bits/ 64 <192000000>;

[Severity: High]
Is there a typo in the UFS reference clock frequency for opp-2? The frequency
is set to 192000000 (192 MHz) instead of the standard 19200000 (19.2 MHz) used
in opp-1. This will cause the OPP framework to incorrectly configure the
reference clock, likely preventing the PHY from locking or causing the
controller to fail.

> +				};
> +			};
> +		};

[ ... ]

> diff --git a/drivers/ufs/host/ufs-spacemit.c b/drivers/ufs/host/ufs-spacemit.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/ufs/host/ufs-spacemit.c

[ ... ]

> +static int ufs_spacemit_check_hibern8(struct ufs_hba *hba)
> +{
> +	u32 tx_fsm_val_0 = 0;
> +	u32 tx_fsm_val_1 = 0;
> +	int retries = DIV_ROUND_UP(HBRN8_POLL_TOUT_MS * 1000, 100);
> +	int err = 0;
> +
> +	do {
> +		err = ufshcd_dme_get(hba,
> +				     UIC_ARG_MIB_SEL(MPHY_TX_FSM_STATE,
> +						     UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),
> +				     &tx_fsm_val_0);
> +		err |= ufshcd_dme_get(hba,
> +				      UIC_ARG_MIB_SEL(MPHY_TX_FSM_STATE,
> +						      UIC_ARG_MPHY_TX_GEN_SEL_INDEX(1)),
> +				      &tx_fsm_val_1);

[Severity: Low]
Does this bitwise OR corrupt the error code if ufshcd_dme_get() fails? Since
ufshcd_dme_get() returns standard negative integer error codes on failure,
combining them with a bitwise OR will result in a nonsensical value, which is
later printed in the kernel log.

[ ... ]

> +/**
> + * ufs_spacemit_event_notify - Handle UFS error events
> + * @hba: host controller instance
> + * @evt: event type
> + * @data: event-specific data
> + *
> + * Handles error events from UFS core, dumps registers immediately
> + * and schedules FSM state dump for later execution in workqueue context.
> + */

[Severity: Low]
Can we update the documentation or the implementation? The docstring states this
function schedules an FSM state dump for later execution in a workqueue context,
but the implementation only prints a warning message and takes no further action.

> +static void ufs_spacemit_event_notify(struct ufs_hba *hba, enum ufs_event_type evt, void *data)

[ ... ]

> +static void ufs_spacemit_post_hibern8(struct ufs_hba *hba, enum uic_cmd_dme cmd)
> +{
> +	static const struct ufshcd_dme_attr_val setup_attrs[] = {
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x84 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x84 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x80 },
> +		{ UIC_ARG_MIB_SEL(0xf1, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x80 },
> +		{ UIC_ARG_MIB(0xdd), 0x57 },
> +		{ UIC_ARG_MIB(0xdd), 0xd7 },
> +		{ UIC_ARG_MIB(0xe8), 0x57 },
> +		{ UIC_ARG_MIB(0xe8), 0xd7 },
> +	};
> +
> +	if (cmd == UIC_CMD_DME_HIBER_ENTER) {
> +		ufs_spacemit_check_hibern8(hba);
> +
> +		ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
> +
> +		/* Power down M-PHY */
> +		ufshcd_writel(hba, 0x0, UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL);
> +
> +		/* Keep reference clock enabled, assert device reset */
> +		ufshcd_writel(hba, MPHY_DEVICE_RESET_ASSERT,
> +			      UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);

[Severity: High]
Does this code actively hard-reset the UFS device when entering the Hibern8
suspend state? Writing MPHY_DEVICE_RESET_ASSERT asserts the RST_N pin, which
wipes all internal state in the hardware.

When exiting Hibern8, the core will issue a DME_HIBER_EXIT command, but since
the device has been reset, it will require a full DME_LINK_STARTUP sequence
instead. This mismatch will cause the resume process to fail.

> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-08-k3-ufs-support-v2-0-45036a2a2418@kernel.org?part=3