Re: [PATCH 4/7] phy: allwinner: a523: add USB3/PCIe PHY driver

[Philipp Zabel <p.zabel@pengutronix.de>]

On Sa, 2025-08-16 at 16:46 +0800, iuncuim wrote:
> From: Mikhail Kalashnikov <iuncuim@gmail.com>
> 
> The A523 family of processors features a combophy for USB 3.0 and PCIe,
> developed by Innosilicon. Simultaneous operation of both interfaces is
> not supported by design.
> Currently, the driver only adds support for USB 3.0. PCIe support is
> currently unavailable and will be added later.
> All data on phy configuration is taken from the manufacturer's BSP driver.
> 
> Signed-off-by: Mikhail Kalashnikov <iuncuim@gmail.com>
> ---
>  drivers/phy/allwinner/Kconfig                |   9 +
>  drivers/phy/allwinner/Makefile               |   1 +
>  drivers/phy/allwinner/phy-sun55i-usb3-pcie.c | 267 +++++++++++++++++++
>  3 files changed, 277 insertions(+)
>  create mode 100644 drivers/phy/allwinner/phy-sun55i-usb3-pcie.c
> 
> diff --git a/drivers/phy/allwinner/Kconfig b/drivers/phy/allwinner/Kconfig
> index fb584518b..af2a82e51 100644
> --- a/drivers/phy/allwinner/Kconfig
> +++ b/drivers/phy/allwinner/Kconfig
> @@ -57,3 +57,12 @@ config PHY_SUN50I_USB3
>  	  part of Allwinner H6 SoC.
>  
>  	  This driver controls each individual USB 2+3 host PHY combo.
> +
> +config PHY_SUN55I_USB3_PCIE
> +	tristate "Allwinner A523 Innosilicon USB3/PCIe Combophy Driver"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	depends on RESET_CONTROLLER

This dependency is not necessary. The reset controller API is stubbed
out in case  RESET_CONTROLLER is disabled.

ARCH_SUNXI selects ARCH_HAS_RESET_CONTROLLER, which default-enables
RESET_CONTROLLER.

> +	select GENERIC_PHY
> +	help
> +	  Enable this to support the Allwinner PCIe/USB3.0 combo PHY
> +	  with Innosilicon IP block founded in A523/A527/H728/T527 SOC
> diff --git a/drivers/phy/allwinner/Makefile b/drivers/phy/allwinner/Makefile
> index bd74901a1..5948a27ef 100644
> --- a/drivers/phy/allwinner/Makefile
> +++ b/drivers/phy/allwinner/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
>  obj-$(CONFIG_PHY_SUN6I_MIPI_DPHY)	+= phy-sun6i-mipi-dphy.o
>  obj-$(CONFIG_PHY_SUN9I_USB)		+= phy-sun9i-usb.o
>  obj-$(CONFIG_PHY_SUN50I_USB3)		+= phy-sun50i-usb3.o
> +obj-$(CONFIG_PHY_SUN55I_USB3_PCIE)		+= phy-sun55i-usb3-pcie.o
> diff --git a/drivers/phy/allwinner/phy-sun55i-usb3-pcie.c b/drivers/phy/allwinner/phy-sun55i-usb3-pcie.c
> new file mode 100644
> index 000000000..905c54a67
> --- /dev/null
> +++ b/drivers/phy/allwinner/phy-sun55i-usb3-pcie.c
> @@ -0,0 +1,267 @@
[...]
> +static int sun55i_usb3_pcie_phy_init(struct phy *_phy)
> +{
> +	struct sun55i_usb3_pcie_phy *phy = phy_get_drvdata(_phy);
> +
> +	sun55i_usb3_phy_open(phy);

Given that sun55i_usb3_pcie_phy_exit() asserts the reset and disables
the clock, why isn't the clock enabled and the reset deasserted here?

As is, it looks like the code in sun55i_usb3_phy_open() could be just
folded into sun55i_usb3_pcie_phy_init().

> +	return 0;
> +}
[...]
> +static int sun55i_usb3_pcie_phy_probe(struct platform_device *pdev)
> +{
> +	struct sun55i_usb3_pcie_phy *phy;
> +	struct device *dev = &pdev->dev;
> +	struct phy_provider *phy_provider;
> +	int ret;
> +
> +	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
> +	if (!phy)
> +		return -ENOMEM;
> +
> +	phy->dev = dev;
> +	phy->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(phy->clk)) {
> +		if (PTR_ERR(phy->clk) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get phy clock\n");
> +		return PTR_ERR(phy->clk);
> +	}
> +
> +	phy->reset = devm_reset_control_get(dev, NULL);

Please use devm_reset_control_get_exclusive() directly.

> +	if (IS_ERR(phy->reset)) {
> +		dev_err(dev, "failed to get reset control\n");

Consider using dev_err_probe().

regards
Philipp