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

[Yixun Lan <dlan@kernel.org>]

Hi All,

On 03:12 Thu 06 Aug     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Failure to enter HIBERN8 state is ignored before forcibly powering down the M-PHY.
> - [High] Missing DME attributes configuration for TX lane 1 and inconsistent MIB selector usage.
> - [Medium] The retrieved connected TX lanes value is silently discarded.
> - [Low] Redundant and ineffective call to `ufshcd_vops_phy_initialization`.
> - [Low] The device tree references an undefined clock macro `CLK_APMU_UFS_REFCLK`, which causes a Device Tree compilation failure.
> --
> 
> --- Patch [3]: [PATCH v3 3/3] riscv: dts: spacemit: k3: Add UFS support ---
> commit 7b2ecae3ba0ff920d65fce0475cce3fc5045f08d
> Author: Yixun Lan <dlan@kernel.org>
> Subject: riscv: dts: spacemit: k3: Add UFS support
> 
> Add UFS Host Controller support for SpacemiT K3 SoC, and enable
> it both on Pico-ITX and CoM260-IFX boards.
> 
> > diff --git a/arch/riscv/boot/dts/spacemit/k3.dtsi b/arch/riscv/boot/dts/spacemit/k3.dtsi
> > index 19fc9b49668ed..cee1569608ea2 100644
> > --- a/arch/riscv/boot/dts/spacemit/k3.dtsi
> > +++ b/arch/riscv/boot/dts/spacemit/k3.dtsi
> > @@ -469,6 +469,28 @@ 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 reference an undefined clock macro?
> 
> The device tree references CLK_APMU_UFS_REFCLK, but this macro appears to be
> undefined. Will this cause a compilation failure when building the device tree?
> 
this should be solved in v7.3-rc1, which ufs clock driver has been merged

> [ ... ]
> 
> > diff --git a/drivers/ufs/host/ufs-spacemit.c b/drivers/ufs/host/ufs-spacemit.c
> > new file mode 100644
> > index 0000000000000..94942ea27fb7d
> > --- /dev/null
> > +++ b/drivers/ufs/host/ufs-spacemit.c
> > @@ -0,0 +1,687 @@
> 
> [ ... ]
> 
> > +static int ufs_spacemit_uniprov1p6_init(struct ufs_hba *hba)
> > +{
> > +	static const struct ufshcd_dme_attr_val setup_attrs[] = {
> 
> [ ... ]
> 
> > +		/* TX HB8_TIME CAP */
> > +		{ UIC_ARG_MIB_SEL(0x000F, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0x64 },
> > +		{ UIC_ARG_MIB_SEL(0x000F, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(1)), 0x64 },
> > +		/* RX HB8_TIME CAP */
> > +		{ UIC_ARG_MIB_SEL(0x0092, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(0)), 0x64 },
> > +		{ UIC_ARG_MIB_SEL(0x0092, UIC_ARG_MPHY_RX_GEN_SEL_INDEX(1)), 0x64 },
> > +		/* TX EQ 3DB */
> > +		{ UIC_ARG_MIB_SEL(0x00CD, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),  0x5 },
> 
> [Severity: High]
> Are we missing the TX EQ 3DB configuration for lane 1 here?
> 
> The device tree specifies lanes-per-direction = <2>, but this DME attribute
> is only configured for TX lane 0. Could this leave lane 1 with undefined or
> default parameters and prevent successful 2-lane initialization?
> 
No need, false report..
The configuration ally to lane 1 too
> [ ... ]
> 
> > +static int ufs_spacemit_link_startup_post_change(struct ufs_hba *hba)
> > +{
> > +	/* Add 0xe8 make UFS2.1 run GEAR3 + 2Lane@409M */
> > +	static const struct ufshcd_dme_attr_val setup_attrs[] = {
> > +		{ UIC_ARG_MIB_SEL(0xe8, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0x97 },
> > +		{ UIC_ARG_MIB_SEL(0xe8, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0xd7 },
> > +		{ UIC_ARG_MIB_SEL(0xe8, UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)), 0x17 },
> 
> [Severity: High]
> Is the DME configuration for TX lane 1 missing?
> 
> Similar to ufs_spacemit_uniprov1p6_init(), attribute 0xe8 is explicitly
> targeting only lane 0. Does lane 1 also need to be configured for 2-lane mode?
> 
no need, false report
same reason as above

> > +		{ UIC_ARG_MIB(DL_AFC0REQTIMEOUTVAL), UFS_DL_AFC0REQTIMEOUTVAL_MAX },
> > +	};
> > +	u32 tx_lanes;
> > +	int err;
> > +
> > +	err = ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
> > +	if (err < 0)
> > +		return err;
> > +
> > +	return ufs_spacemit_get_connected_tx_lanes(hba, &tx_lanes);
> 
> [Severity: Medium]
> Is the retrieved tx_lanes value meant to be used?
> 
> The value of tx_lanes is populated by ufs_spacemit_get_connected_tx_lanes()
> but is then silently discarded when returning. Should this value be verified
> against the configuration?
> 
tx_lanes is not used, I will drop this

> [ ... ]
> 
> > +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);
> 
> [Severity: High]
> Should we check the return value of ufs_spacemit_check_hibern8(hba) before
> powering down the M-PHY?
> 
will check return value, and give a warning 

> If the HIBERN8 check times out and returns -ETIMEDOUT, the error is ignored
> and the driver unconditionally powers down the PHY. Can this cause a bus lockup
> or hardware error if the M-PHY isn't actually in the HIBERN8 state?
> 
I'm not sure what's the best way to deal with this.. any suggestion?

> [ ... ]
> 
> > +static int ufs_spacemit_init(struct ufs_hba *hba)
> > +{
> > +	int err = 0;
> > +	struct device *dev = hba->dev;
> > +	struct ufs_spacemit_host *host;
> > +	struct reset_control *rst;
> > +
> > +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> > +	if (!host)
> > +		return -ENOMEM;
> > +
> > +	rst = devm_reset_control_get_exclusive_deasserted(dev, NULL);
> > +	if (IS_ERR(rst))
> > +		return dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n");
> > +
> > +	host->hba = hba;
> > +	ufshcd_set_variant(hba, host);
> > +	ufs_spacemit_advertise_quirks(hba);
> > +
> > +	err = ufshcd_vops_phy_initialization(host->hba);
> > +	return err;
> > +}
> 
> [Severity: Low]
> Is this call to ufshcd_vops_phy_initialization() effective?
> 
> ufs_hba_spacemit_vops does not define the phy_initialization callback, so
> the wrapper function will silently return 0. Does this need to be called here,
> or is it redundant since the real PHY initialization occurs in
> ufs_spacemit_link_startup_pre_change()?

Indeed, there is no phy_initialization(), I will drop this code
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260806-08-k3-ufs-support-v3-0-037fb322acdb@kernel.org?part=3

-- 
Yixun Lan (dlan)