RE: [PATCH v2 2/3] phy: add AST2700 usb3.2 phy driver

[Ryan Chen]

> Subject: Re: [PATCH v2 2/3] phy: add AST2700 usb3.2 phy driver
> 
> Hi Ryan,
> 
> On Fri, 2026-01-16 at 10:53 +0800, Ryan Chen wrote:
> 
> 
> ...
> 
> > diff --git a/drivers/phy/aspeed/phy-aspeed-usb3.c
> > b/drivers/phy/aspeed/phy-aspeed-usb3.c
> > new file mode 100644
> > index 000000000000..872d2163fcf5
> > --- /dev/null
> > +++ b/drivers/phy/aspeed/phy-aspeed-usb3.c
> > @@ -0,0 +1,236 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2026 Aspeed Technology Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +
> > +#define PHY3S00		0x00
> > +#define PHY3S00_INIT_DONE		BIT(15)
> > +#define PHY3S00_SRAM_BYPASS		BIT(7)
> > +#define PHY3S00_SRAM_EXT_LOAD	BIT(6)
> >
> 
> ...
> 
> > +
> > +static int aspeed_usb3_phy_init(struct phy *phy) {
> > +	struct aspeed_usb3_phy *aspeed_phy = phy_get_drvdata(phy);
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(aspeed_phy->clk);
> > +	if (ret) {
> > +		dev_err(aspeed_phy->dev, "Failed to enable clock %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = reset_control_deassert(aspeed_phy->rst);
> > +	if (ret) {
> > +		clk_disable_unprepare(aspeed_phy->clk);
> > +		return ret;
> 
> Nit: Given we have to do this below if the reset_control_deassert() succeeds,
> perhaps add a label below and use goto here?

Will update

	ret = reset_control_deassert(aspeed_phy->rst);
	if (ret)
		goto err_disable_clk;
	...
	if (ret) {
		dev_err(aspeed_phy->dev, "SRAM init timeout\n");
		goto err_assert_reset;
	}
	...
	return 0;

err_assert_reset:
	reset_control_assert(aspeed_phy->rst);
err_disable_clk:
	clk_disable_unprepare(aspeed_phy->clk);
	return ret;


> 
> > +	}
> > +
> > +	/* Wait for USB3 PHY internal SRAM initialization done */
> > +	ret = readl_poll_timeout(aspeed_phy->regs + PHY3S00, val,
> > +				 val & PHY3S00_INIT_DONE,
> > +				 USEC_PER_MSEC, 10 * USEC_PER_MSEC);
> > +	if (ret) {
> > +		dev_err(aspeed_phy->dev, "SRAM init timeout\n");
> > +		goto err_assert_reset;
> > +	}
> > +
> > +	val = readl(aspeed_phy->regs + PHY3S00);
> > +	val |= PHY3S00_SRAM_BYPASS;
> > +	writel(val, aspeed_phy->regs + PHY3S00);
> 
> According to the datasheet PHY3S00[15] (PHY3S00_INIT_DONE above)
> indicates that the PHY internal SRAM initialisation is complete. The datasheet
> reports the SRAM is used for configuration of calibration among other things.
> PHY3S00[6] instructs the PHY that software has completed loading the
> configuration data into SRAM, however PHY3S00_SRAM_BYPASS (PHY3S00[7])
> tells the PHY to load configuration from "hard wired" values.
> 
> Is it necessary to wait for SRAM initialisation to complete if we're bypassing it?
> Or are there other side-effects involved in the setting of PHY3S00[15]?

Yes, it is necessary to wait SRAM initial, the driver polls PHY3S00[15].
It reports that the boot loader in the PCS has finished initialising the 
SRAM (loading the contents into the PCS), and that initialisation has 
to complete before sram_bypass (PHY3S00[7]) may be asserted. 


> 
> > +
> > +	/* Set protocol1_ext signals as default PHY3 settings based on SNPS
> documents.
> > +	 * Including PCFGI[54]: protocol1_ext_rx_los_lfps_en for better
> compatibility
> > +	 */
> > +	writel(PHY3P00_DEFAULT, aspeed_phy->regs + PHY3P00);
> > +	writel(PHY3P04_DEFAULT, aspeed_phy->regs + PHY3P04);
> > +	writel(PHY3P08_DEFAULT, aspeed_phy->regs + PHY3P08);
> > +	writel(PHY3P0C_DEFAULT, aspeed_phy->regs + PHY3P0C);
> > +
> > +	return 0;
> > +
> > +err_assert_reset:
> > +	reset_control_assert(aspeed_phy->rst);
> > +	clk_disable_unprepare(aspeed_phy->clk);
> > +	return ret;
> > +}
> >
> 
> ...
> 
> >
> > +static struct platform_driver aspeed_usb3_phy_driver = {
> > +	.probe		= aspeed_usb3_phy_probe,
> > +	.driver		= {
> > +		.name	= KBUILD_MODNAME,
> > +		.of_match_table	= aspeed_usb3_phy_match_table,
> > +	},
> > +};
> > +module_platform_driver(aspeed_usb3_phy_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("ASPEED USB3.0 PHY Driver");
> 
> MODULE_AUTHOR()?
Will update
MODULE_AUTHOR("Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>");