Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Vendor M-PHY TX attributes are only configured for TX Lane 0, neglecting Lane 1 in a 2-lane setup.
- [High] Hardcoded dual-lane check in `ufs_spacemit_check_hibern8` will fail and timeout on 1-lane UFS devices.
- [High] The UFS hardware reset pulse width is too short, violating the JEDEC UFS specification.
- [Medium] Misleading macro names and comments for device reset contradict hardware behavior.
- [Medium] Silently ignoring DME configuration errors.
- [Low] Usage of `of_match_ptr` causes build warnings when `CONFIG_OF` is disabled.
--
--- Patch [2]: [PATCH v3 2/3] scsi: ufs: spacemit: k3: Add UFS Host Controller driver ---
commit a6bb8f511d306cddb9d21e41123fafdbcd63a6c9
Author: Yixun Lan <[email protected]>
Subject: scsi: ufs: spacemit: k3: Add UFS Host Controller driver
SpacemiT K3 SoC consist of UFS (Universal Flash Storage) Host Controller
which has features compatible with JEDEC UFS 2.2, MIPI UniPro v1.61 and
M-PHY v3.0 standard.
> 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
[ ... ]
> +/* PHY register magic values */
> +#define MPHY_PU_ALL 0x87f
> +#define MPHY_PU_WITH_HB8_RESET 0xb7f
> +#define MPHY_DEVICE_RESET_DEASSERT 0x101
> +#define MPHY_DEVICE_RESET_ASSERT 0x001
[Severity: Medium]
Is this macro name misleading? Looking at ufs_spacemit_device_reset(), it
uses 0x000 to assert the reset and 0x101 to de-assert it. This means 0x001
actually disables the reference clock while keeping the reset de-asserted.
[ ... ]
> +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);
> + if (err)
> + break;
> +
> + 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);
> + if (err || (tx_fsm_val_0 == TX_FSM_HIBERN8 &&
> + tx_fsm_val_1 == TX_FSM_HIBERN8))
> + break;
[Severity: High]
Will this hardcoded dual-lane check timeout on single-lane UFS devices?
If a 1-lane memory chip is connected, Lane 1 is inactive. Querying its state
will either return a DME error or read a non-Hibern8 state, causing the loop
to timeout after 100ms and break power management.
Could this check be updated to use the dynamically negotiated lane count from
dev_req_params to know how many lanes to poll?
[ ... ]
> +static int ufs_spacemit_uniprov1p6_init(struct ufs_hba *hba)
> +{
> + static const struct ufshcd_dme_attr_val setup_attrs[] = {
> + { UIC_ARG_MIB(0x1552), 0x4f }, /* PA_TXHSG1SYNCLENGTH */
[ ... ]
> + /* 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]
Should this TX equalization be applied to Lane 1 as well?
The host controller is configured for 2 lanes, but this setup attribute only
targets UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0). Omitting tuning parameters for the
second lane could cause asymmetric lane configurations or link instability.
[ ... ]
> +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]
Does attribute 0xe8 need to be programmed for both lanes?
Similar to the TX equalization setup, this only programs Lane 0 despite the
driver specifying dual-lane support.
[ ... ]
> +static int ufs_spacemit_apply_dev_quirks(struct ufs_hba *hba)
> +{
> + static const struct ufshcd_dme_attr_val setup_attrs[] = {
[ ... ]
> + };
> +
> + if (hba->dev_quirks & UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME)
> + ufs_spacemit_quirk_host_pa_saveconfigtime(hba);
> +
> + if (hba->dev_info.wmanufacturerid == UFS_VENDOR_WDC)
> + hba->dev_quirks |= UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE;
> +
> + ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
[Severity: Medium]
Is it safe to ignore the return values of these DME configuration functions?
If ufs_spacemit_quirk_host_pa_saveconfigtime() or ufs_spacemit_dme_set()
fail, the errors are not propagated. Could this allow initialization to proceed
with an incorrectly configured M-PHY?
[ ... ]
> +static int ufs_spacemit_device_reset(struct ufs_hba *hba)
> +{
> + /* Stop device ref_clk & asserted ufs device reset */
> + ufshcd_writel(hba, 0x000, UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);
> + usleep_range(10, 15);
[Severity: High]
Is this reset pulse width long enough?
The JEDEC UFS specification mandates that the host must maintain the RST_n
signal low for a minimum of 1 ms. A 10 to 15 microsecond delay might cause
the memory chip to fail to reset properly.
[ ... ]
> +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 },
[Severity: High]
Are these attributes defaulting to Lane 0?
Using UIC_ARG_MIB() defaults to Lane 0. If these need to be applied to Lane 1
as well during post-Hibern8 configuration, they are currently missing.
[ ... ]
> + 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: Medium]
Does this operation contradict its comment?
The code writes MPHY_DEVICE_RESET_ASSERT (0x001), which disables the reference
clock but keeps the reset de-asserted. The comment says "Keep reference clock
enabled, assert device reset", which is the exact opposite.
While keeping reset de-asserted is the correct action for Hibern8 (asserting
reset would destroy the device's state), the comment and macro name suggest
incorrect hardware behavior.
[ ... ]
> +static struct platform_driver ufs_spacemit_pltform = {
> + .probe = ufs_spacemit_probe,
> + .remove = ufs_spacemit_remove,
> + .driver = {
> + .name = "ufshcd-spacemit",
> + .of_match_table = of_match_ptr(ufs_spacemit_of_match),
> + },
> +};
[Severity: Low]
Does the use of of_match_ptr() here cause a compiler warning?
Because ufs_spacemit_of_match is defined unconditionally, building with
CONFIG_OF disabled will evaluate of_match_ptr() to NULL, leaving the array
unused and triggering a -Wunused-variable warning.
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.