Re: [PATCH v7 2/2] phy: cix: Add CIX Sky1 USB3 PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] phy: cix: redundant locking, refcounting, and wrappers in
  phy-cix-usb3.c
- [Low] phy: cix: unused 4-lane and non-0803 register macros in
  phy-cix-usb3.h
--

commit 3b4a169b7338f7dde4e66109a72c7946bef8ea14
Author: Hongliang Yang <hongliang.yang@cixtech.com>

phy: cix: Add CIX Sky1 USB3 PHY driver

This commit adds a driver for the Cadence SD0803 USB3 PHY integrated in
the CIX Sky1 SoC.

> diff --git a/drivers/phy/cix/phy-cix-usb3.c b/drivers/phy/cix/phy-cix-usb3.c
> new file mode 100644
> index 0000000000000..c6376f787af03
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-usb3.c

[ ... ]

> +struct cix_u3phy {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct regmap *phy_regmap;
> +	struct reset_control *preset;
> +	struct reset_control *reset;
> +	struct clk *apb_clk;
> +	struct clk *ref_clk;
> +	struct mutex mutex; /* mutex to protect access to individual PHYs */
> +	bool init;
> +	int init_count;
> +	const struct cix_u3phy_cfg *cfg;
> +};
> +
> +static int sky1_u3phy_exit(struct cix_u3phy *u3phy)
> +{
> +	if (WARN_ON(!u3phy->init_count))
> +		return -EINVAL;
> +
> +	u3phy->init_count--;
> +	if (u3phy->init && u3phy->init_count == 0) {
> +		reset_control_assert(u3phy->reset);
> +		reset_control_assert(u3phy->preset);
> +		clk_disable_unprepare(u3phy->ref_clk);
> +		clk_disable_unprepare(u3phy->apb_clk);
> +		u3phy->init = false;
> +	}
> +	return 0;
> +}
> +
> +static int sky1_u3phy_init(struct cix_u3phy *u3phy)
> +{
> +	int ret;
> +
> +	if (!u3phy->init) {
> +		/* usb rcsu reset is default deassert */
> +		reset_control_assert(u3phy->reset);
> +		reset_control_assert(u3phy->preset);
> +		ret = clk_prepare_enable(u3phy->apb_clk);
> +		if (ret) {
> +			dev_err(u3phy->dev, "Failed to prepare_enable u3phy apb clock\n");
> +			return ret;
> +		}
> +		ret = clk_prepare_enable(u3phy->ref_clk);
> +		if (ret) {
> +			dev_err(u3phy->dev, "Failed to prepare_enable u3phy ref clock\n");
> +			goto disable_apb_clk;
> +		}
> +		reset_control_deassert(u3phy->preset);
> +		ret = regmap_multi_reg_write(u3phy->phy_regmap, sky1_u3phy_conf,
> +					     ARRAY_SIZE(sky1_u3phy_conf));
> +		if (ret) {
> +			dev_err(u3phy->dev, "Failed to write the reg sequence\n");
> +			goto assert_reset_preset;
> +		}
> +		u3phy->init = true;
> +		reset_control_deassert(u3phy->reset);
> +	}
> +	u3phy->init_count++;
> +	return 0;
> +
> +assert_reset_preset:
> +	reset_control_assert(u3phy->preset);
> +	reset_control_assert(u3phy->reset);
> +	clk_disable_unprepare(u3phy->ref_clk);
> +disable_apb_clk:
> +	clk_disable_unprepare(u3phy->apb_clk);
> +	return ret;
> +}

[ ... ]

> +static int cix_usb3_phy_power_on(struct cix_u3phy *u3phy)
> +{
> +	int ret;
> +	const struct cix_u3phy_cfg *phy_cfgs = u3phy->cfg;
> +
> +	if (phy_cfgs->u3phy_init) {
> +		ret = phy_cfgs->u3phy_init(u3phy);
> +		if (ret) {
> +			dev_err(u3phy->dev, "failed to init udphy\n");
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int cix_usb3_phy_power_off(struct cix_u3phy *u3phy)
> +{
> +	int ret;
> +	const struct cix_u3phy_cfg *phy_cfgs = u3phy->cfg;
> +
> +	if (phy_cfgs->u3phy_exit) {
> +		ret = phy_cfgs->u3phy_exit(u3phy);
> +		if (ret) {
> +			dev_err(u3phy->dev, "failed to exit udphy\n");
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int cix_u3phy_init(struct phy *phy)
> +{
> +	struct cix_u3phy *u3phy = phy_get_drvdata(phy);
> +	int ret = 0;
> +
> +	mutex_lock(&u3phy->mutex);
> +	ret = cix_usb3_phy_power_on(u3phy);
> +	mutex_unlock(&u3phy->mutex);
> +	return ret;
> +}
> +
> +static int cix_u3phy_exit(struct phy *phy)
> +{
> +	struct cix_u3phy *u3phy = phy_get_drvdata(phy);
> +	int ret = 0;
> +
> +	mutex_lock(&u3phy->mutex);
> +	ret = cix_usb3_phy_power_off(u3phy);
> +	mutex_unlock(&u3phy->mutex);
> +	return ret;
> +}

[Severity: Low]
This isn't a bug, but since cix_u3phy_probe() registers a single struct
phy via of_phy_simple_xlate(), phy_init() and phy_exit() in the PHY core
already serialize calls under phy->mutex and refcount phy->init_count,
calling phy->ops->init() only when phy->init_count == 0 and
phy->ops->exit() only when phy->init_count == 1.

That makes u3phy->mutex, u3phy->init, and u3phy->init_count redundant.
Dropping them and wiring sky1_u3phy_init() and sky1_u3phy_exit() directly
into cix_u3phy_ops would also remove the cix_usb3_phy_power_on(),
cix_usb3_phy_power_off(), and struct cix_u3phy_cfg wrappers, which log
duplicate error messages using the stale "udphy" name.

[ ... ]

> diff --git a/drivers/phy/cix/phy-cix-usb3.h b/drivers/phy/cix/phy-cix-usb3.h
> new file mode 100644
> index 0000000000000..22666cd3b2537
> --- /dev/null
> +++ b/drivers/phy/cix/phy-cix-usb3.h

[ ... ]

> +#define RX_SDCAL0_INIT_TMR_LANE0     0x8044
> +#define RX_SDCAL0_INIT_TMR_LANE1     0x8244
> +#define RX_SDCAL0_INIT_TMR_LANE2     0x8444
> +#define RX_SDCAL0_INIT_TMR_LANE3     0x8644

[ ... ]

> +#define PHY_PIPE_USB3_GEN2_PRE_CFG0  0xc018
> +#define PHY_PIPE_USB3_GEN2_POST_CFG0 0xc01a
> +#define PHY_PIPE_USB3_GEN2_POST_CFG1 0xc01b

[ ... ]

> +/* Bits */
> +
> +#define PHY_PLL_CFG_0803              0xC00E
> +/* 0803 phy register */
> +#define PHY_PIPE_USB3_GEN2_PRE_CFG0_0803	0xc01c
> +#define PHY_PIPE_USB3_GEN2_POST_CFG0_0803	0xc01e
> +#define PHY_PIPE_USB3_GEN2_POST_CFG1_0803	0xc01f
> +
> +#endif

[Severity: Low]
This isn't a bug, but over 70 macros in this private header are unused,
including all *_LANE2 and *_LANE3 definitions for this 2-lane PHY and the
non-0803 PHY_PIPE_USB3_GEN2_* offsets superseded by the *_0803 macros at
the bottom.  Could the unused macros be dropped and the remaining
definitions moved into phy-cix-usb3.c?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111030937.2954646-1-hongliang.yang@cixtech.com?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy