Re: [PATCH 10/13] phy: airoha: Add support for Airoha AN7581 USB PHY

[Vinod Koul <vkoul@kernel.org>]

On 09-03-25, 14:29, Christian Marangi wrote:
> 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.
> 
> On xLate probe, the Serdes mode is validated and the driver return error
> if the Serdes mode doesn't reflect the expected mode. This is required
> as Serdes mode are controlled by the SCU SSR bits and can be either USB
> 3.0 mode or HSGMII or PCIe 2. In such case USB 3.0 won't work.
> 
> If the USB 3.0 mode is not supported, the modes needs to be also
> disabled in the xHCI node or the driver will report unsable clock and
> fail probe.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  MAINTAINERS                         |   1 +
>  drivers/phy/Kconfig                 |   1 +
>  drivers/phy/Makefile                |   3 +-
>  drivers/phy/airoha/Kconfig          |  13 +
>  drivers/phy/airoha/Makefile         |   3 +
>  drivers/phy/airoha/phy-airoha-usb.c | 554 ++++++++++++++++++++++++++++
>  6 files changed, 574 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/phy/airoha/Kconfig
>  create mode 100644 drivers/phy/airoha/Makefile
>  create mode 100644 drivers/phy/airoha/phy-airoha-usb.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c2dd385e9165..1835e488ccaa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -758,6 +758,7 @@ M:	Christian Marangi <ansuelsmth@gmail.com>
>  L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/phy/airoha,an7581-usb-phy.yam
> +F:	drivers/phy/airoha/phy-airoha-usb.c
>  
>  AIRSPY MEDIA DRIVER
>  L:	linux-media@vger.kernel.org
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 8d58efe998ec..19c9c518fc3d 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -93,6 +93,7 @@ config PHY_NXP_PTN3222
>  	  schemes. It supports all three USB 2.0 data rates: Low Speed, Full
>  	  Speed and High Speed.
>  
> +source "drivers/phy/airoha/Kconfig"
>  source "drivers/phy/allwinner/Kconfig"
>  source "drivers/phy/amlogic/Kconfig"
>  source "drivers/phy/broadcom/Kconfig"
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index e281442acc75..71708c6865b8 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -12,7 +12,8 @@ obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
>  obj-$(CONFIG_USB_LGM_PHY)		+= phy-lgm-usb.o
>  obj-$(CONFIG_PHY_AIROHA_PCIE)		+= phy-airoha-pcie.o
>  obj-$(CONFIG_PHY_NXP_PTN3222)		+= phy-nxp-ptn3222.o
> -obj-y					+= allwinner/	\
> +obj-y					+= airoha/	\

Directory for a single driver? Also move the PCIe driver in, if you
really want to do this

> +struct airoha_usb_phy_priv {
> +	struct device *dev;
> +
> +	struct regmap *regmap;
> +
> +	unsigned int id;
> +
> +	struct airoha_usb_phy_instance *phys[AIROHA_USB_PHY_MAX_INSTANCE];

No need for empty lines here?

> +static int airoha_usb_phy_u2_slew_rate_calibration(struct airoha_usb_phy_priv *priv)
> +{
> +	u32 fm_out;
> +	u32 srctrl;
> +
> +	/* Enable HS TX SR calibration */
> +	regmap_set_bits(priv->regmap, AIROHA_USB_PHY_USBPHYACR5,
> +			AIROHA_USB_PHY_USB20_HSTX_SRCAL_EN);
> +
> +	usleep_range(1000, 1500);
> +
> +	/* Enable Free run clock */
> +	regmap_set_bits(priv->regmap, AIROHA_USB_PHY_FMMONR1,
> +			AIROHA_USB_PHY_FRCK_EN);
> +
> +	/* Select Monitor Clock */
> +	regmap_update_bits(priv->regmap, AIROHA_USB_PHY_FMCR0,
> +			   AIROHA_USB_PHY_MONCLK_SEL,
> +			   priv->id == 0 ? AIROHA_USB_PHY_MONCLK_SEL0 :
> +					   AIROHA_USB_PHY_MONCLK_SEL1);
> +
> +	/* Set cyclecnt */
> +	regmap_update_bits(priv->regmap, AIROHA_USB_PHY_FMCR0,
> +			   AIROHA_USB_PHY_CYCLECNT,
> +			   FIELD_PREP(AIROHA_USB_PHY_CYCLECNT,
> +				      AIROHA_USB_PHY_U2_FM_DET_CYCLE_CNT));
> +
> +	/* Enable Frequency meter */
> +	regmap_set_bits(priv->regmap, AIROHA_USB_PHY_FMCR0,
> +			AIROHA_USB_PHY_FREQDET_EN);
> +
> +	/* 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) {
> +		srctrl = 0x5;

magic?

> +		dev_err(priv->dev, "Frequency not detected, using default SR calibration.\n");
> +	/* (1024 / FM_OUT) * REF_CK * U2_SR_COEF (round to the nearest digits) */
> +	} else {
> +		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);
> +	}
> +
> +	regmap_update_bits(priv->regmap, AIROHA_USB_PHY_USBPHYACR5,
> +			   AIROHA_USB_PHY_USB20_HSTX_SRCTRL,
> +			   FIELD_PREP(AIROHA_USB_PHY_USB20_HSTX_SRCTRL, srctrl));
> +
> +	return 0;

This is the only return... consider making it void return then?
Here and other places

> +static int airoha_usb_phy_exit(struct phy *phy)
> +{
> +	return 0;
> +}

you can drop this

> +static int airoha_usb_phy_power_on(struct phy *phy)
> +{
> +	struct airoha_usb_phy_instance *instance = phy_get_drvdata(phy);
> +	struct airoha_usb_phy_priv *priv = dev_get_drvdata(phy->dev.parent);
> +
> +	if (instance->type == PHY_TYPE_USB2)
> +		return airoha_usb_phy_u2_power_on(priv);
> +
> +	return airoha_usb_phy_u3_power_on(priv);

for non USB2, you power both why?

> +static int airoha_usb_phy_u2_set_mode(struct airoha_usb_phy_priv *priv,
> +				      enum phy_mode mode)
> +{
> +	u32 val = 0;
> +
> +	/*
> +	 * 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;
> +		fallthrough;
> +	case PHY_MODE_USB_HOST:
> +		val |= AIROHA_USB_PHY_FORCE_IDDIG;
> +		break;
> +	case PHY_MODE_USB_OTG:
> +		break;

Not expecting to set this?
-- 
~Vinod