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

From: Yixun Lan <dlan@kernel.org>
To: Troy Mitchell <troy.mitchell@linux.spacemit.com>
Cc: Alim Akhtar <alim.akhtar@samsung.com>,
	Avri Altman <avri.altman@sandisk.com>,
	Bart Van Assche <bvanassche@acm.org>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Paul Walmsley <pjw@kernel.org>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Alexandre Ghiti <alex@ghiti.fr>,
	"Martin K. Petersen" <mkp@kernel.org>,
	Anirudh Srinivasan <asrinivasan@oss.tenstorrent.com>,
	Aurelien Jarno <aurelien@aurel32.net>,
	Jennifer Berringer <jberring@redhat.com>,
	linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
	linux-riscv@lists.infradead.org, spacemit@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 2/2] scsi: ufs: spacemit: k3: Add UFS Host Controller driver
Date: Fri, 2 Oct 2026 02:59:52 +0000	[thread overview]
Message-ID: <20261002025952-GKB51329@kernel.org> (raw)
In-Reply-To: <DLLQJDEA91FU.18Q1TSX09AUR3@linux.spacemit.com>

Hi Troy,

On 17:25 Tue 22 Sep     , Troy Mitchell wrote:
> On Mon Sep 21, 2026 at 8:36 PM +08, Yixun Lan wrote:
> > [...]
> >
> > +static int ufs_spacemit_link_startup_pre_change(struct ufs_hba *hba)
> > +{
> > +	u32 value, sys1clk_1us;
> > +	int err;
> > +
> > +	err = ufs_spacemit_mphy_init(hba);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	err = ufs_spacemit_uniprov1p6_init(hba);
> > +	if (err < 0)
> > +		return err;
> > +
> 
> Should this be `if (err)`? The call chain is:
> 
>   ufs_spacemit_uniprov1p6_init()
>     -> ufs_spacemit_dme_set()
>       -> ufshcd_dme_set()
>         -> ufshcd_dme_set_attr()
> 
> ufshcd_dme_set_attr() returns 0 on success and non-zero on failure,
the comment of ufshcd_dme_set_attr() says so,

> including positive UIC error codes. The current check misses those.
> 
but as I checked the code, there is no positive err code, so I don't
think it's necessary to change

> Also, ufshcd_link_startup() ignores the PRE_CHANGE return value in this
> series' base, so returning an error here alone will not abort startup.
> 
I think it's totally fine for it to ignore, as there are follow-up
checks, as I see, it's function of link_startup_notify() that returning
the error code

> > [...]
> >
> > +static int ufs_spacemit_apply_dev_quirks(struct ufs_hba *hba)
> > +{
> > [...]
> > +	err = ufs_spacemit_dme_set(hba, setup_attrs, ARRAY_SIZE(setup_attrs));
> > +	if (err < 0)
> > +		return err;
> The same `if (err)` check is needed here for positive DME errors.
> 
ditto
> > +
> > +	return ufs_spacemit_wait_mphy_pll_lock(hba);
> > +}
> > +
> > [...]
> >
> > +static int ufs_spacemit_init(struct ufs_hba *hba)
> > +{
> > [...]
> > +	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> > +
> > +	return 0;
> > +}
> > +
> 

-- 
Yixun Lan (dlan)

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