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.
The issue here is that, while both QCOM and SAMSUNG use that IP, a lot of
the registers are not mapped for Exynoses.
For example with QCOM:
``
#define USB_PHY_UTMI_CTRL0 (0x3c)
#define PHY_UTMI_CTRL5 (0x50)
``
Exynoses:
``
/* nothing before this */
#define EXYNOS2200_EUSB2_RST_CTRL 0x0
``
Here EXYNOS2200_EUSB2_RST_CTRL seems to be the same register as
QCOM_PHY_UTMI_CTRL5. Another instance is:
``
#define UTMI_PHY_CMN_CTRL0 (0x98)
#define TESTBURNIN BIT(6)
``
Exynoses:
``
#define EXYNOS2200_EUSB2_TESTSE (0x20)
#define EXYNOS2200_TEST_IDDQ BIT(6)
``
But at the same time there are some.. inconsistencies between them.
Looking at the register layout for the exynos implementation for TX tuning
the following register offset and bits are described:
``
/*
* Offset : 0x0014
* Description: tune register of tx driver
*/
typedef union {
u32 data;
struct {
// bit[0] :
unsigned fsls_slew_rate : 1;
// bit[2:1] :
unsigned fsls_vref_tune : 2;
// bit[3] :
unsigned fsls_vreg_bypass : 1;
// bit[6:4]
unsigned hs_vref_tune : 3;
// bit [8:7]
unsigned hs_xv : 2;
// bit [11:9]
unsigned preemp : 3;
// bit [13:12]
unsigned res : 2;
// bit [15:14]
unsigned rise : 2;
// bit[31:16]
unsigned RSVD31_16 : 16;
} b;
} EUSBCON_REG_TXTUNE_o, *EUSBPHY_REG_TXTUNE_p;
``
And for QCOM the latter functionality is split into two separate register
offsets:
``
#define USB_PHY_CFG_CTRL_8 (0x78)
#define PHY_CFG_TX_FSLS_VREF_TUNE_MASK GENMASK(1, 0)
#define PHY_CFG_TX_FSLS_VREG_BYPASS BIT(2)
#define PHY_CFG_TX_HS_VREF_TUNE_MASK GENMASK(5, 3)
#define PHY_CFG_TX_HS_XV_TUNE_MASK GENMASK(7, 6)
#define USB_PHY_CFG_CTRL_9 (0x7c)
#define PHY_CFG_TX_PREEMP_TUNE_MASK GENMASK(2, 0)
#define PHY_CFG_TX_RES_TUNE_MASK GENMASK(4, 3)
#define PHY_CFG_TX_RISE_TUNE_MASK GENMASK(6, 5)
#define PHY_CFG_RCAL_BYPASS BIT(7)
``
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 - 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
