Re: [PATCH v7 6/6] phy: airoha: Add support for Airoha AN7581 USB PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] `dev_err_probe` is called with `IS_ERR()` instead of `PTR_ERR()`, breaking error propagation and probe deferral.
- [High] Incorrect configuration of `FORCE_IDDIG` in `PHY_MODE_USB_DEVICE` due to a missing flag.
- [Medium] `fm_out` may be used uninitialized if `regmap_read_poll_timeout` fails.
- [Medium] Missing `select REGMAP_MMIO` dependency in Kconfig.
- [Low] Dead code in `an7581_usb_phy_power_on` checking the USB PHY's mode instead of the SerDes PHY's mode.
- [Low] Assigning generic `PHY_TYPE_USB2` values to a variable typed as a local internal enum.
--

commit a54da44fe952522b06761e81fb643efff0dad77a
Author: Christian Marangi <ansuelsmth@gmail.com>

phy: airoha: Add support for Airoha AN7581 USB PHY

Add support for Airoha AN7581 USB PHY driver. AN7581 supports up to 2
USB port with USB 2.0 mode always supported and USB 3.0 mode available
only if the Serdes port is correctly configured for USB 3.0.

> diff --git a/drivers/phy/airoha/Kconfig b/drivers/phy/airoha/Kconfig
> index 9a1b625a77012..bb4e3367baa58 100644
> --- a/drivers/phy/airoha/Kconfig
> +++ b/drivers/phy/airoha/Kconfig
[ ... ]
> @@ -11,3 +11,13 @@ config PHY_AIROHA_AN7581_PCIE
>  	  Say Y here to add support for Airoha AN7581 PCIe PHY driver.
>  	  This driver create the basic PHY instance and provides initialize
>  	  callback for PCIe GEN3 port.
> +
> +config PHY_AIROHA_AN7581_USB
> +	tristate "Airoha AN7581 USB PHY Driver"
> +	depends on ARCH_AIROHA || COMPILE_TEST
> +	depends on OF
> +	select GENERIC_PHY

[Severity: Medium]
Will this fail to link under COMPILE_TEST on architectures without REGMAP_MMIO?
The driver relies on devm_regmap_init_mmio(), so it might need to explicitly
select REGMAP_MMIO.

> +	help
> +	  Say 'Y' here to add support for Airoha AN7581 USB PHY driver.
> +	  This driver create the basic PHY instance and provides initialize
> +	  callback for USB port.
> diff --git a/drivers/phy/airoha/phy-an7581-usb.c b/drivers/phy/airoha/phy-an7581-usb.c
> new file mode 100644
> index 0000000000000..90fd2cbe68d42
> --- /dev/null
> +++ b/drivers/phy/airoha/phy-an7581-usb.c
[ ... ]
> +static void an7581_usb_phy_u2_slew_rate_calibration(struct an7581_usb_phy_priv *priv)
> +{
> +	u32 fm_out;
> +	u32 srctrl;
[ ... ]
> +	/* Timeout can happen and we will apply workaround at the end */
> +	regmap_read_poll_timeout(priv->regmap, AIROHA_USB_PHY_FMMONR0, fm_out,
> +				 fm_out, AIROHA_USB_PHY_FREQDET_SLEEP,
> +				 AIROHA_USB_PHY_FREQDET_TIMEOUT);
> +
> +	/* Disable Frequency meter */
> +	regmap_clear_bits(priv->regmap, AIROHA_USB_PHY_FMCR0,
> +			  AIROHA_USB_PHY_FREQDET_EN);
> +
> +	/* Disable Free run clock */
> +	regmap_clear_bits(priv->regmap, AIROHA_USB_PHY_FMMONR1,
> +			  AIROHA_USB_PHY_FRCK_EN);
> +
> +	/* Disable HS TX SR calibration */
> +	regmap_clear_bits(priv->regmap, AIROHA_USB_PHY_USBPHYACR5,
> +			  AIROHA_USB_PHY_USB20_HSTX_SRCAL_EN);
> +
> +	usleep_range(1000, 1500);
> +
> +	/* Frequency was not detected, use default SR calibration value */
> +	if (!fm_out) {

[Severity: Medium]
If the underlying regmap_read() fails immediately (e.g., due to an I/O error),
does fm_out remain uninitialized?

Since the return value of regmap_read_poll_timeout() isn't checked, an
uninitialized stack value could bypass this check and cause unpredictable
behavior during the subsequent division.

> +		srctrl = AIROHA_USB_PHY_DEFAULT_SR_CALIBRATION;
> +		dev_err(priv->dev, "Frequency not detected, using default SR calibration.\n");
> +	} else {
> +		/* (1024 / FM_OUT) * REF_CK * U2_SR_COEF (round to the nearest digits) */
> +		srctrl = AIROHA_USB_PHY_REF_CK * AIROHA_USB_PHY_U2_SR_COEF;
> +		srctrl = (srctrl * AIROHA_USB_PHY_U2_FM_DET_CYCLE_CNT) / fm_out;
> +		srctrl = DIV_ROUND_CLOSEST(srctrl, AIROHA_USB_PHY_U2_SR_COEF_DIVISOR);
> +		dev_dbg(priv->dev, "SR calibration applied: %x\n", srctrl);
> +	}
[ ... ]
> +static int an7581_usb_phy_power_on(struct phy *phy)
> +{
> +	struct an7581_usb_phy_instance *instance = phy_get_drvdata(phy);
> +	struct an7581_usb_phy_priv *priv = dev_get_drvdata(phy->dev.parent);
> +
> +	switch (instance->type) {
> +	case PHY_TYPE_USB2:
> +		an7581_usb_phy_u2_power_on(priv);
> +		break;
> +	case PHY_TYPE_USB3:
> +		if (phy_get_mode(phy) == PHY_MODE_PCIE ||
> +		    phy_get_mode(phy) == PHY_MODE_ETHERNET)
> +			return 0;

[Severity: Low]
Is this check examining the wrong phy object?

It seems to be checking the USB PHY's mode instead of the shared SerDes combo
PHY (priv->serdes_phy). Since the USB PHY's mode will presumably never be PCIe
or Ethernet, will this condition ever evaluate to true?

> +
> +		an7581_usb_phy_u3_power_on(priv);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int an7581_usb_phy_power_off(struct phy *phy)
> +{
> +	struct an7581_usb_phy_instance *instance = phy_get_drvdata(phy);
> +	struct an7581_usb_phy_priv *priv = dev_get_drvdata(phy->dev.parent);
> +
> +	switch (instance->type) {
> +	case PHY_TYPE_USB2:
> +		an7581_usb_phy_u2_power_off(priv);
> +		break;
> +	case PHY_TYPE_USB3:
> +		if (phy_get_mode(phy) == PHY_MODE_PCIE ||
> +		    phy_get_mode(phy) == PHY_MODE_ETHERNET)
> +			return 0;

[Severity: Low]
Similar to the an7581_usb_phy_power_on() check above, does this check the
wrong phy object?

> +
> +		an7581_usb_phy_u3_power_off(priv);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int an7581_usb_phy_u2_set_mode(struct an7581_usb_phy_priv *priv,
> +				      enum phy_mode mode)
> +{
> +	u32 val;
> +
> +	/*
> +	 * For Device and Host mode, enable force IDDIG.
> +	 * For Device set IDDIG, for Host clear IDDIG.
> +	 * For OTG disable force and clear IDDIG bit while at it.
> +	 */
> +	switch (mode) {
> +	case PHY_MODE_USB_DEVICE:
> +		val = AIROHA_USB_PHY_IDDIG;

[Severity: High]
The comment mentions enabling force IDDIG for Device mode, but val is only
assigned AIROHA_USB_PHY_IDDIG here. Should AIROHA_USB_PHY_FORCE_IDDIG be
included as well?

> +		break;
> +	case PHY_MODE_USB_HOST:
> +		val = AIROHA_USB_PHY_FORCE_IDDIG |
> +		      AIROHA_USB_PHY_FORCE_IDDIG;

[Severity: Low]
This isn't a bug, but the AIROHA_USB_PHY_FORCE_IDDIG flag is duplicated here.

> +		break;
> +	case PHY_MODE_USB_OTG:
> +		val = 0;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	regmap_update_bits(priv->regmap, AIROHA_USB_PHY_U2PHYDTM1,
> +			   AIROHA_USB_PHY_FORCE_IDDIG |
> +			   AIROHA_USB_PHY_IDDIG, val);
> +
> +	return 0;
> +}
[ ... ]
> +static int an7581_usb_phy_probe(struct platform_device *pdev)
> +{
> +	struct phy_provider *phy_provider;
> +	struct an7581_usb_phy_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	unsigned int index;
> +	void __iomem *base;
> +	int ret;
[ ... ]
> +	for (index = 0; index < AIROHA_PHY_USB_MAX; index++) {
> +		enum an7581_usb_phy_instance_type phy_type;
> +		struct an7581_usb_phy_instance *instance;
> +
> +		switch (index) {
> +		case AIROHA_PHY_USB2:
> +			phy_type = PHY_TYPE_USB2;
> +			break;
> +		case AIROHA_PHY_USB3:
> +			phy_type = PHY_TYPE_USB3;
> +			break;
> +		}

[Severity: Low]
Should phy_type be declared as an integer or u32?

The generic PHY_TYPE_USB2 macro evaluates to 3, but phy_type is typed as
enum an7581_usb_phy_instance_type which only defines values 0 to 2.

> +
> +		if (phy_type == PHY_TYPE_USB3) {
> +			priv->serdes_phy = devm_phy_get(dev, NULL);
> +			if (IS_ERR(priv->serdes_phy))
> +				return dev_err_probe(dev, IS_ERR(priv->serdes_phy), "missing serdes phy for USB 3.0\n");

[Severity: High]
Does this inadvertently return 1 instead of a proper negative error code?

Since IS_ERR() returns a boolean, returning it breaks error propagation and
probe deferral (e.g. -EPROBE_DEFER). Should this be PTR_ERR() instead?

> +		}
> +
> +		instance = devm_kzalloc(dev, sizeof(*instance), GFP_KERNEL);
> +		if (!instance)
> +			return -ENOMEM;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519220813.28468-1-ansuelsmth@gmail.com?part=6