RE: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920

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

Hi Neil,

Thank you for reviewing the patches.

> -----Original Message-----
> From: neil.armstrong@linaro.org <neil.armstrong@linaro.org>
> Sent: 20 May 2025 01:10 PM
> To: Pritam Manohar Sutar <pritam.sutar@samsung.com>; vkoul@kernel.org;
> 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;
> m.szyprowski@samsung.com; s.nawrocki@samsung.com
> Cc: 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 v2 2/2] phy: exyons5-usbdrd: support HS phy for
> ExynosAutov920
> 
> On 16/05/2025 12:26, Pritam Manohar Sutar wrote:
> > This SoC has a single USB 3.1 DRD combo phy and three USB2.0 DRD HS
> > phy controllers those only support the UTMI+ interface.
> >
> > Support only UTMI+ for this SoC which is very similar to what the
> > existing Exynos850 supports.
> >
> > The combo phy supports both UTMI+ (HS) and PIPE3 (SS) and is out of
> > scope of this commit.
> >
> > Add required change in phy driver to support HS phy for this SoC.
> >
> > Signed-off-by: Pritam Manohar Sutar <pritam.sutar@samsung.com>
> > ---
> >   drivers/phy/samsung/phy-exynos5-usbdrd.c | 85
> ++++++++++++++++++++++++
> >   1 file changed, 85 insertions(+)
> >
> > diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > index 634c4310c660..b440b56c6595 100644
> > --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > @@ -177,6 +177,9 @@
> >   #define HSPHYPLLTUNE_PLL_P_TUNE			GENMASK(3, 0)
> >
> >   /* Exynos850: USB DRD PHY registers */
> > +#define EXYNOSAUTOv920_DRD_CTRL_VER		0x00
> > +#define CTRL_VER_MAJOR_VERSION			GENMASK(31, 24)
> > +
> >   #define EXYNOS850_DRD_LINKCTRL			0x04
> >   #define LINKCTRL_FORCE_RXELECIDLE		BIT(18)
> >   #define LINKCTRL_FORCE_PHYSTATUS		BIT(17)
> > @@ -1772,6 +1775,10 @@ static const char * const
> exynos5_regulator_names[] = {
> >   	"vbus", "vbus-boost",
> >   };
> >
> > +static const char * const exynosautov920_clk_names[] = {
> > +	"ext_xtal",
> > +};
> > +
> >   static const struct exynos5_usbdrd_phy_drvdata exynos5420_usbdrd_phy = {
> >   	.phy_cfg		= phy_cfg_exynos5,
> >   	.phy_ops		= &exynos5_usbdrd_phy_ops,
> > @@ -1847,6 +1854,81 @@ static const struct exynos5_usbdrd_phy_drvdata
> exynos850_usbdrd_phy = {
> >   	.n_regulators		= ARRAY_SIZE(exynos5_regulator_names),
> >   };
> >
> > +static void exynosautov920_usbdrd_utmi_init(struct exynos5_usbdrd_phy
> > +*phy_drd) {
> > +	u32 version;
> > +
> > +	version = readl(phy_drd->reg_phy +
> EXYNOSAUTOv920_DRD_CTRL_VER);
> > +	dev_info(phy_drd->dev, "usbphy: version:0x%x\n", version);
> 
> Please do not add mode info to boot log, use dev_dbg instead.

Will replace dev_info by dev_dbg.

> 
> > +
> > +	if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> > +		/* utmi init for exynosautov920 HS phy */
> > +		exynos850_usbdrd_utmi_init(phy_drd);
> > +}
> > +
> > +static int exynosautov920_usbdrd_phy_init(struct phy *phy) {
> > +	struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > +	struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> > +	int ret = 0;
> > +
> > +	ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd-
> >clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* UTMI or PIPE3 specific init */
> > +	inst->phy_cfg->phy_init(phy_drd);
> > +
> > +	clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks,
> > +phy_drd->clks);
> > +
> > +	return 0;
> > +}
> > +
> > +static void exynosautov920_v3p1_phy_dis(struct phy *phy) {
> > +	struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > +	struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> > +	void __iomem *reg_phy = phy_drd->reg_phy;
> > +	u32 version;
> > +
> > +	version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
> > +
> > +	if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> > +		exynos850_usbdrd_phy_exit(phy);
> > +}
> > +
> > +static int exynosautov920_usbdrd_phy_exit(struct phy *phy) {
> > +	struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > +
> > +	if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
> > +		exynosautov920_v3p1_phy_dis(phy);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
> > +	.init		= exynosautov920_usbdrd_phy_init,
> > +	.exit		= exynosautov920_usbdrd_phy_exit,
> 
> <snip>
> 
> > +		.id		= EXYNOS5_DRDPHY_UTMI,
> > +		.phy_init	= exynosautov920_usbdrd_utmi_init,
> 
> <snip>
> 
> > +	}, {
> > +		.compatible = "samsung,exynosautov920-usb31drd-phy",
> > +		.data = &exynosautov920_usb31drd_phy
> 
> All those new ops are only called when matching this compatible, it it really
> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
> phy" PHYs with version different from 3 in the wild ?
> 

This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently, 
supporting only v303 phys in this patch-set, and planning v400 phy later (soon). 

Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.

> Neil
> 
> >   	},
> >   	{ },
> >   };


Thank you,
Pritam