Re: [PATCH] phy: phy-snps-eusb2: fix FSEL_48_MHZ_VAL definition

[Xu Yang]

On Tue, Sep 08, 2026 at 04:17:57PM +0400, ivaylo wrote:
> On 8.09.26 12:59, Xu Yang wrote:
> > From: Xu Yang <xu.yang_2@xxxxxxx>
> > 
> > According to Synopsys eUSB2 PHY Databook, FSEL_48_MHZ_VAL should be 0x6
> > for supported 48 MHz reference clock.
> 
> I don't see this as a justified change. Samsung's implementation of eUSB2
> differs from the generic Synopsys interface, as is visible from the
> differences
> between QCOM's implementation and Samsung's.
> 
> From what I see for exynos2200, the FSEL value is 0x2 for both 24 MHz and
> 48 MHz. The differentiating part is that the upper PLL divider is 0x0 for 24
> MHz
> and 0x1 for 48 MHz. The current programming was taken from the samsung\

For Product Code H483-0, it's 0x2 for 24 MHz and 0x6 for 48 MHz. The PLL divider
is 0x0 for 24 MHz and 0x1 for 48 MHz too. So it does seem like something mismatch
in your side. Because the FSEL_*_MHZ_VAL is common definition in the driver, this
means FSEL_48_MHZ_VAL is not common anymore. But if you have confirmed it's simply
like that, it's fine. Others need to add their specific definition in the future.

> vendor kernel driver, which is a reliable-ish reference for the differences
> in register
> interface implementation when we have no other public resource like a
> manual.
> 
> ..now that I look into it though, the 11_8 value should probably be
> field_prep'd
> instead of just directly passed as an argument of
> snps_eusb2_hsphy_write_mask(),
> because right now it's essentially 0x1 & 0xf00 = 0? I haven't tested this
> path on
> my hardware, so it might prove to be a regression in the future?
> So:
> snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG1,
> EXYNOS_PHY_CFG_PLL_FB_DIV_11_8_MASK,
> FIELD_PREP(EXYNOS_PHY_CFG_PLL_FB_DIV_11_8_MASK, config->div_11_8_val));

Yeah, sashiko had reported it:
https://sashiko.dev/#/patchset/20260908085953.3315953-1-xu.yang_2@xxxxxxxxxxx?part=1

Thanks,
Xu Yang

> 
> For this patch though, unless this change is tested on real hardware and
> backed up
> by a technical reference manual for this particular implementation, CC-ing
> the
> change to stable is unreasonable.
> 
> Best regards,
> Ivaylo Ivanov
> 
> > 
> > Fixes: c4098f3e6134 ("phy: phy-snps-eusb2: add support for exynos2200")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> > ---
> >   drivers/phy/phy-snps-eusb2.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/phy/phy-snps-eusb2.c b/drivers/phy/phy-snps-eusb2.c
> > index af4fa17ac6cb..3cb18d6ebc1d 100644
> > --- a/drivers/phy/phy-snps-eusb2.c
> > +++ b/drivers/phy/phy-snps-eusb2.c
> > @@ -21,7 +21,7 @@
> >   #define FSEL_20_MHZ_VAL			(0x1)
> >   #define FSEL_24_MHZ_VAL			(0x2)
> >   #define FSEL_26_MHZ_VAL			(0x3)
> > -#define FSEL_48_MHZ_VAL			(0x2)
> > +#define FSEL_48_MHZ_VAL			(0x6)
> >   #define EXYNOS_USB_PHY_CFG_PLLCFG0	(0x8)
> >   #define PHY_CFG_PLL_FB_DIV_19_8_MASK	GENMASK(19, 8)
>