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

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

On 2/16/25 15:57, Ivaylo Ivanov wrote:
> On 2/16/25 15:19, Krzysztof Kozlowski wrote:
>> On 16/02/2025 10:51, Ivaylo Ivanov wrote:
>>>>>>  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?
>> For example: one common module and two modules per each soc, because I
>> assume some per-soc stuff might be needed. But maybe even these two
>> modules are not necessary and everything could be in one driver.
...
>
> So, Exynos2200 has a much simpler eusb initialization sequence than what
> is present in mainline for QCOMs. I still don't really think the drivers
> should be merged, as we aren't really duplicating code per-say.
>
> I've already started working on merging them, and my current idea is to
> not redefine the registers once again for 2200, but rather make an enum
> that defines if the SoC is a QCOM or EXYNOS, and select the register
> offsets dynamically

Never mind. That's a bad idea - after more digging way too much bits differ
not just the register layout. I'll implement the init/exit sequence in the
qcom driver separately. Sadly I can't reuse much code.

Best regards,
Ivaylo

>  - similarly as how I did with USIv1. If a register
> offset is not present, it'd just not do the write. My guess is that this
> will make it work with the qualcomm init sequence as well, so it'd result
> in even less redundant code (apart from the eUSB tuning, which can be
> omitted for now).
>
>>> What about the current modelling scheme, as-in taking the phandle to
>>> the usbcon phy and handling it?
>> What about it? 
> As I said in the commit description, I'm passing the USBCON phy as a
> phandle to the eusb2 node and enabling/disabling it when needed. I'm
> not 100% sure it would be adequate to include that in a common snps EUSB
> driver, as it seems to more of a quirk with the exynoses. But then how
> can I model it so that it's correctly described according to how the
> hardware works (as-in usbcon "muxing" between child phys, in this case
> eUSB and snps USBDP combophy)
>
> Regarding repeaters, I still don't have the TI repeater implemented.
>
> Best regards,
> Ivaylo
>
>> Did you look at the bindings of qcom snps eusb2? Are you
>> saying you do not have here repeater? If so, then this phy phandle might
>> not be correct.
>>
>>
>>
>> Best regards,
>> Krzysztof