Re: [PATCH v1 3/4] phy: samsung: add Exynos2200 SNPS eUSB2 driver

[Ivaylo Ivanov]

On 2/16/25 11:26, Krzysztof Kozlowski wrote:
> On 15/02/2025 13:24, Ivaylo Ivanov wrote:
>> The Exynos2200 SoC uses Synopsis eUSB2 PHY for USB 2.0. Add a new
>> driver for it.
>>
>> eUSB2 on Exynos SoCs is usually paired alongside a USB PHY controller.
>> Currently the driver is modelled to take and enable/disable the usb phy
>> controller when needed.
>>
>> The driver is based on information from downstream drivers.
>>
>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@xxxxxxxxx>
>> ---
>>  drivers/phy/samsung/Kconfig                   |  13 +
>>  drivers/phy/samsung/Makefile                  |   1 +
>>  .../phy/samsung/phy-exynos2200-snps-eusb2.c   | 351 ++++++++++++++++++
>>  3 files changed, 365 insertions(+)
>>  create mode 100644 drivers/phy/samsung/phy-exynos2200-snps-eusb2.c
>>
>> diff --git a/drivers/phy/samsung/Kconfig b/drivers/phy/samsung/Kconfig
>> index e2330b089..f62285254 100644
>> --- a/drivers/phy/samsung/Kconfig
>> +++ b/drivers/phy/samsung/Kconfig
>> @@ -77,6 +77,19 @@ config PHY_S5PV210_USB2
>>  	  particular SoC is compiled in the driver. In case of S5PV210 two phys
>>  	  are available - device and host.
>>  
>> +config PHY_EXYNOS2200_SNPS_EUSB2
>> +	tristate "Exynos2200 eUSB 2.0 PHY driver"
>> +	depends on (ARCH_EXYNOS && OF) || COMPILE_TEST
>> +	depends on HAS_IOMEM
>> +	depends on USB_DWC3_EXYNOS
>
> How does it depend? What are you using from DWC3?

Can drop, I guess.

>
>> +	select GENERIC_PHY
>> +	select MFD_SYSCON
> Where do you use it?

Remained from USBCON driver.

>
>> +	default y
>> +	help
>> +	  Enable USBCON PHY support for Exynos2200 SoC.
>> +	  This driver provides PHY interface for eUSB 2.0 controller
>> +	  present on Exynos5 SoC series.
>> +
>>  config PHY_EXYNOS5_USBDRD
>>  	tristate "Exynos5 SoC series USB DRD PHY driver"
>>  	depends on (ARCH_EXYNOS && OF) || COMPILE_TEST
>> diff --git a/drivers/phy/samsung/Makefile b/drivers/phy/samsung/Makefile
>> index fea1f96d0..90b84c7fc 100644
>> --- a/drivers/phy/samsung/Makefile
>> +++ b/drivers/phy/samsung/Makefile
>> @@ -14,5 +14,6 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
>>  phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4X12_USB2)	+= phy-exynos4x12-usb2.o
>>  phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2)	+= phy-exynos5250-usb2.o
>>  phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2)	+= phy-s5pv210-usb2.o
>> +obj-$(CONFIG_PHY_EXYNOS2200_SNPS_EUSB2)	+= phy-exynos2200-snps-eusb2.o
> Entire driver looks like repeating existing qcom-snps-eusb2.

It's the same IP, but implemented differently on a different platform. At
the very least, the register layout is different.

>  You need to
> integrate the changes, not create duplicated driver.

I can do that, but it would be come a bit cluttered, won't it? Depends on
if we want to follow the current oem-provided initialization sequence, or
try and fully reuse what we have in there.

Best regards,
Ivaylo

>
> ...
>
>> +
>> +	ret = devm_clk_bulk_get(dev, drv_data->n_clks,
>> +				phy->clks);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "failed to get phy clock(s)\n");
>> +
>> +	for (int i = 0; i < phy->drv_data->n_clks; ++i) {
>> +		if (!strcmp(phy->clks[i].id, "ref")) {
>> +			phy->ref_clk = phy->clks[i].clk;
>> +			break;
>> +		}
>> +	}
>> +
>> +	phy->vregs = devm_kcalloc(dev, drv_data->n_regulators,
>> +				  sizeof(*phy->vregs), GFP_KERNEL);
>> +	if (!phy->vregs)
>> +		return -ENOMEM;
>> +	regulator_bulk_set_supply_names(phy->vregs,
>> +					drv_data->regulator_names,
>> +					drv_data->n_regulators);
>> +	ret = devm_regulator_bulk_get(dev, drv_data->n_regulators,
>> +				      phy->vregs);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
>> +
>> +	/* we treat the usblink controller phy as a separate phy */
>> +	phy->usbcon = devm_of_phy_get_by_index(dev, np, 0);
>> +	if (IS_ERR(phy->usbcon))
>> +		return dev_err_probe(dev, PTR_ERR(phy->usbcon),
>> +				     "failed to get usbcon\n");
>> +
>> +	generic_phy = devm_phy_create(dev, NULL, &exynos2200_snps_eusb2_phy_ops);
>> +	if (IS_ERR(generic_phy)) {
>> +		dev_err(dev, "failed to create phy %d\n", ret);
>
> Syntax is return dev_err_probe
>
>> +		return PTR_ERR(generic_phy);
>> +	}
>> +
>> +	dev_set_drvdata(dev, phy);
>> +	phy_set_drvdata(generic_phy, phy);
>> +
>> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>> +	if (IS_ERR(phy_provider)) {
>> +		dev_err(dev, "failed to register phy provider\n");
>
> Syntax is return dev_err_probe
>
>> +		return PTR_ERR(phy_provider);
>> +	};
>> +
>> +	return 0;
>> +}
> Best regards,
> Krzysztof