Re: [PATCH 2/3] scsi: ufs: spacemit: k3: Add UFS Host Controller driver

[Bart Van Assche <bvanassche@acm.org>]

On 7/1/26 7:31 PM, Yixun Lan wrote:
> +config SCSI_UFS_SPACEMIT
> +	tristate "SpacemiT UFS controller driver"
> +	depends on SCSI_UFSHCD_PLATFORM && ARCH_SPACEMIT

What would break if ARCH_SPACEMIT is left out?

Please make sure that this driver can be compile-tested easily.

> +	u32 host_reg[] = {
> +		(UFS_PHY_MNG_BASE + UFS_MPHY_RST_CTRL),
> +		(UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL),
> +		(UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL),
> +		0xFFF,
> +	};

Can this array be declared 'static const'?

The parentheses in the above array definition are superfluous.
Please remove these.

> +	buf = kzalloc(VENDOR_DUMP_BUF_SIZE, GFP_ATOMIC);

Why GFP_ATOMIC? ufs_spacemit_dump_host_regs() is never called from
atomic context, isn't it?

> +	len += scnprintf(buf + len, VENDOR_DUMP_BUF_SIZE - len, "vendor specific registers:");

This function will be much easier to review and to maintain if a struct
seq_buf instance is created for 'buf' and if seq_buf_printf() would be
used instead of scnprintf().

> +static bool is_fsm_state_valid(u32 state)
> +{
> +	return (state == FSM_STATE_ACTIVE || state == FSM_STATE_LS_BURST);
> +}

"return" is not a function. Please remove the superfluous parentheses.

> +static int ufs_spacemit_mphy_init(struct ufs_hba *hba)
> +{
> +	int ret;
> +
> +	/* reset all mphy logical */
> +	ufshcd_writel(hba, 0x003, UFS_PHY_MNG_BASE + 0x0);
> +	mdelay(1);

Why mdelay() instead of msleep()?

> +static int ufs_spacemit_uniprov1p6_init(struct ufs_hba *hba)
> +{
> +	/* PA_TXHSG1SYNCLENGTH */
> +	ufshcd_dme_set(hba, UIC_ARG_MIB(0x1552), 0x4f);
> +
> +	/* PA_TXHSG1PREPARELENGTH */
> +	ufshcd_dme_set(hba, UIC_ARG_MIB(0x1553), 0xf);

The code in this function is very repetitive. Please convert all the
ufshcd_dme_set() calls into a loop over an array.

> +	/*LCC_DISABLE*/

Here and everywhere else in this source file, please follow the
convention used elsewhere in the Linux kernel and change /*comment*/
into /* comment */.

> +/**
> + * ufs_spacemit_hibern8_notify - Handle hibernate enter/exit
> + * @hba: host controller instance
> + * @cmd: UIC command (HIBER_ENTER or HIBER_EXIT)
> + * @status: notification status
> + *
> + * Manages M-PHY power state during hibernate transitions.
> + */
> +static void ufs_spacemit_hibern8_notify(struct ufs_hba *hba, enum uic_cmd_dme cmd,
> +					enum ufs_notify_change_status status)
> +{
> +	int ret;
> +
> +	dev_dbg(hba->dev, "Hibern8 notify: cmd=%d, status=%d\n", cmd, status);
> +	if (status == PRE_CHANGE) {
> +		if (cmd == UIC_CMD_DME_HIBER_EXIT) {
> +			mdelay(1);
> +
> +			/* Enable reference clock */
> +			ufshcd_writel(hba, MPHY_DEVICE_RESET_DEASSERT,
> +				      UFS_PHY_MNG_BASE + UFS_DEVICE_IO_CTRL);
> +			mdelay(1);
> +
> +			/* Power up all */
> +			ufshcd_writel(hba, MPHY_PU_ALL, UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL);
> +			mdelay(1);
> +
> +			/* Assert ana_rx_hb8_reset */
> +			ufshcd_writel(hba, MPHY_PU_WITH_HB8_RESET,
> +				      UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL);
> +			mdelay(1);
> +
> +			/* Deassert ana_rx_hb8_reset */
> +			ufshcd_writel(hba, MPHY_PU_ALL, UFS_PHY_MNG_BASE + UFS_MPHY_PU_CTRL);
> +
> +			ret = ufs_spacemit_wait_mphy_pll_lock(hba);
> +			if (ret < 0)
> +				return;
> +
> +			mdelay(1);
> +			ufshcd_dme_set(hba, UIC_ARG_MIB(0xdd), 0x57);
> +			mdelay(1);
> +			ufshcd_dme_set(hba, UIC_ARG_MIB(0xe8), 0x57);
> +		}
> +	}

Please move all the code in this function that is indented by two tabs 
into new functions to improve code readability and to reduce
indentation. This advice comes from the Linux kernel coding style guide.

Thanks,

Bart.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv