RE: [PATCH v5 4/6] phy: exynos5-usbdrd: support HS combo phy for ExynosAutov920

["Pritam Manohar Sutar" <pritam.sutar@samsung.com>]

Hi Vinod,

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: 12 August 2025 07:48 PM
> To: Pritam Manohar Sutar <pritam.sutar@samsung.com>
> Cc: kishon@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; alim.akhtar@samsung.com; andre.draszik@linaro.org;
> peter.griffin@linaro.org; kauschluss@disroot.org;
> ivo.ivanov.ivanov1@gmail.com; igor.belwon@mentallysanemainliners.org;
> m.szyprowski@samsung.com; s.nawrocki@samsung.com; linux-
> phy@lists.infradead.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
linux-samsung-
> soc@vger.kernel.org; rosa.pila@samsung.com; dev.tailor@samsung.com;
> faraz.ata@samsung.com; muhammed.ali@samsung.com;
> selvarasu.g@samsung.com
> Subject: Re: [PATCH v5 4/6] phy: exynos5-usbdrd: support HS combo phy for
> ExynosAutov920
> 
> On 05-08-25, 17:22, Pritam Manohar Sutar wrote:
> > Support UTMI+ combo phy for this SoC which is somewhat simmilar to
> > what the existing Exynos850 support does. The difference is that some
> > register offsets and bit fields are defferent from Exynos850.
> >
> > Add required change in phy driver to support combo HS phy for this SoC.
> >
> > Signed-off-by: Pritam Manohar Sutar <pritam.sutar@samsung.com>
> > ---
> >  drivers/phy/samsung/phy-exynos5-usbdrd.c | 210
> > +++++++++++++++++++++++
> >  1 file changed, 210 insertions(+)
> >
> > diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > index 5400dd23e500..c22f4de7d094 100644
> > --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > @@ -41,6 +41,13 @@
> >  #define EXYNOS2200_CLKRST_LINK_PCLK_SEL		BIT(1)
> >
> >  #define EXYNOS2200_DRD_UTMI			0x10
> > +
> > +/* ExynosAutov920 bits */
> > +#define UTMICTL_FORCE_UTMI_SUSPEND		BIT(13)
> > +#define UTMICTL_FORCE_UTMI_SLEEP		BIT(12)
> > +#define UTMICTL_FORCE_DPPULLDOWN		BIT(9)
> > +#define UTMICTL_FORCE_DMPULLDOWN		BIT(8)
> > +
> >  #define EXYNOS2200_UTMI_FORCE_VBUSVALID		BIT(1)
> >  #define EXYNOS2200_UTMI_FORCE_BVALID		BIT(0)
> >
> > @@ -250,6 +257,22 @@
> >  #define EXYNOS850_DRD_HSP_TEST			0x5c
> >  #define HSP_TEST_SIDDQ				BIT(24)
> >
> > +#define EXYNOSAUTOV920_DRD_HSP_CLKRST		0x100
> > +#define HSPCLKRST_PHY20_SW_PORTRESET		BIT(3)
> > +#define HSPCLKRST_PHY20_SW_POR			BIT(1)
> > +#define HSPCLKRST_PHY20_SW_POR_SEL		BIT(0)
> > +
> > +#define EXYNOSAUTOV920_DRD_HSPCTL		0x104
> > +#define HSPCTRL_VBUSVLDEXTSEL			BIT(13)
> > +#define HSPCTRL_VBUSVLDEXT			BIT(12)
> > +#define HSPCTRL_EN_UTMISUSPEND			BIT(9)
> > +#define HSPCTRL_COMMONONN			BIT(8)
> > +
> > +#define EXYNOSAUTOV920_DRD_HSP_TEST		0x10c
> > +
> > +#define EXYNOSAUTOV920_DRD_HSPPLLTUNE		0x110
> > +#define HSPPLLTUNE_FSEL				GENMASK(18, 16)
> > +
> >  /* Exynos9 - GS101 */
> >  #define EXYNOS850_DRD_SECPMACTL			0x48
> >  #define SECPMACTL_PMA_ROPLL_REF_CLK_SEL		GENMASK(13,
> 12)
> > @@ -2054,6 +2077,139 @@ static const struct exynos5_usbdrd_phy_drvdata
> exynos990_usbdrd_phy = {
> >  	.n_regulators		= ARRAY_SIZE(exynos5_regulator_names),
> >  };
> >
> > +static void
> > +exynosautov920_usbdrd_utmi_init(struct exynos5_usbdrd_phy *phy_drd) {
> > +	void __iomem *reg_phy = phy_drd->reg_phy;
> > +	u32 reg;
> > +
> > +	/*
> > +	 * Disable HWACG (hardware auto clock gating control). This
> > +	 * forces QACTIVE signal in Q-Channel interface to HIGH level,
> > +	 * to make sure the PHY clock is not gated by the hardware.
> > +	 */
> > +	reg = readl(reg_phy + EXYNOS850_DRD_LINKCTRL);
> > +	reg |= LINKCTRL_FORCE_QACT;
> > +	writel(reg, reg_phy + EXYNOS850_DRD_LINKCTRL);
> 
> maybe add a read-modify-write helper, this is user a lot here

Used this convention for readability purpose. Other SoCs are also using 
convention in this file. 

Moreover, noted this and will consider to clean-up this file later.

> 
> > +
> > +	/* De-assert link reset */
> > +	reg = readl(reg_phy + EXYNOS2200_DRD_CLKRST);
> > +	reg &= ~CLKRST_LINK_SW_RST;
> > +	writel(reg, reg_phy + EXYNOS2200_DRD_CLKRST);
> > +
> > +	/* Set PHY POR High */
> > +	reg = readl(reg_phy + EXYNOSAUTOV920_DRD_HSP_CLKRST);
> > +	reg |= HSPCLKRST_PHY20_SW_POR | HSPCLKRST_PHY20_SW_POR_SEL;
> > +	writel(reg, reg_phy + EXYNOSAUTOV920_DRD_HSP_CLKRST);
> > +
> > +	/* Enable UTMI+ */
> > +	reg = readl(reg_phy + EXYNOS2200_DRD_UTMI);
> > +	reg &= ~(UTMICTL_FORCE_UTMI_SUSPEND |
> UTMICTL_FORCE_UTMI_SLEEP |
> > +		UTMICTL_FORCE_DPPULLDOWN |
> UTMICTL_FORCE_DMPULLDOWN);
> > +	writel(reg, reg_phy + EXYNOS2200_DRD_UTMI);
> > +
> > +	/* set phy clock & control HS phy */
> > +	reg = readl(reg_phy + EXYNOSAUTOV920_DRD_HSPCTL);
> > +	reg |= HSPCTRL_EN_UTMISUSPEND | HSPCTRL_COMMONONN;
> > +	writel(reg, reg_phy + EXYNOSAUTOV920_DRD_HSPCTL);
> > +
> > +	fsleep(100);
> > +
> > +	/* Set VBUS Valid and DP-Pull up control by VBUS pad usage */
> > +	reg = readl(reg_phy + EXYNOS850_DRD_LINKCTRL);
> > +	reg |= FIELD_PREP_CONST(LINKCTRL_BUS_FILTER_BYPASS, 0xf);
> > +	writel(reg, reg_phy + EXYNOS850_DRD_LINKCTRL);
> > +
> > +	reg = readl(reg_phy + EXYNOS2200_DRD_UTMI);
> > +	reg |= EXYNOS2200_UTMI_FORCE_VBUSVALID |
> EXYNOS2200_UTMI_FORCE_BVALID;
> > +	writel(reg, reg_phy + EXYNOS2200_DRD_UTMI);
> > +
> > +	reg = readl(reg_phy + EXYNOSAUTOV920_DRD_HSPCTL);
> > +	reg |= HSPCTRL_VBUSVLDEXTSEL | HSPCTRL_VBUSVLDEXT;
> > +	writel(reg, reg_phy + EXYNOSAUTOV920_DRD_HSPCTL);
> > +
> > +	/* Setting FSEL for refference clock */
> > +	reg = readl(reg_phy + EXYNOSAUTOV920_DRD_HSPPLLTUNE);
> > +	reg &= ~HSPPLLTUNE_FSEL;
> 
> Empty line here please

Will add empty line.

> 
> > +	switch (phy_drd->extrefclk) {
> > +	case EXYNOS5_FSEL_50MHZ:
> > +		reg |= FIELD_PREP(HSPPLLTUNE_FSEL, 7);
> > +		break;
> > +	case EXYNOS5_FSEL_26MHZ:
> > +		reg |= FIELD_PREP(HSPPLLTUNE_FSEL, 6);
> > +		break;
> > +	case EXYNOS5_FSEL_24MHZ:
> > +		reg |= FIELD_PREP(HSPPLLTUNE_FSEL, 2);
> > +		break;
> > +	case EXYNOS5_FSEL_20MHZ:
> > +		reg |= FIELD_PREP(HSPPLLTUNE_FSEL, 1);
> > +		break;
> > +	case EXYNOS5_FSEL_19MHZ2:
> > +		reg |= FIELD_PREP(HSPPLLTUNE_FSEL, 0);
> > +		break;
> > +	default:
> > +		dev_warn(phy_drd->dev, "unsupported ref clk: %#.2x\n",
> > +			 phy_drd->extrefclk);
> 
> but we still continue?

This SoC supports 19.2Mhz refclk and it sets default reg value for this
refclk if it does not find clk.

> --
> ~Vinod

Thank you,

Regards,
Pritam