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

[Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>]

On 2/16/25 11:44, Krzysztof Kozlowski wrote:
> On 16/02/2025 10:41, Ivaylo Ivanov wrote:
>> 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@gmail.com>
>>>> ---
>>>>  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.
>
> I checked few registers, looked very the same. Same blocks from synopsys
> have the common register layouts.

I see.

>
>>>  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.
>
> I think it duplicates a lot, so it won't be clutter. We have many
> drivers having common code and per-variant ops.

So the approach to take here is to make a common driver?

What about the current modelling scheme, as-in taking the phandle to
the usbcon phy and handling it?

Best regards,
Ivaylo

>
> Best regards,
> Krzysztof