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

[Yixun Lan <dlan@kernel.org>]

Hi Bart,

On 07:01 Mon 06 Jul     , Bart Van Assche wrote:
> 
> 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?
> 
Should be fine, I see no hard build dependency

> Please make sure that this driver can be compile-tested easily.
> 
I could make it "SCSI_UFSHCD_PLATFORM && (ARCH_SPACEMIT || COMPILE_TEST)"

> > +	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'?
> 
Yes, sure

> The parentheses in the above array definition are superfluous.
> Please remove these.
> 
Ok, will do

> > +	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?
> 
No, it will be called from interrupt context

devm_request_threaded_irq(dev, irq, ufshcd_intr, ufshcd_threaded_intr,..)
ufshcd_intr() 
    ufshcd_sl_intr()
         ufshcd_check_errors() 
            ufshcd_update_evt_hist
                 ufshcd_vops_event_notify()
                       ufs_spacemit_event_notify

but I will see if able to move this function to .dbg_register_dump() and
redesign it with ufshcd_dump_regs(), or simply drop it from this version,
 and implement it as a separate patch in future.

P.S, checked ufshcd_dump_regs() also use GFP_ATOMIC for memory allocation,
see drivers/ufs/core/ufshcd.c:174

> > +	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().
> 
should depend on above..

> > +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.
> 
Ok

> > +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()?
> 
Ok, TBO, I need to check more widely about the delay functions,
generally there are too many, too long delay()s ..

some could be simply dropped, some could be reduced,
some could be converted into sleepable function..

> > +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.
> 
Ok

> > +	/*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 */.
> 
Ok

> > +/**
> > + * 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.
> 
Ok, will do

-- 
Yixun Lan (dlan)