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

[ivaylo <ivo.ivanov.ivanov1@gmail.com>]

On 8.09.26 12:59, Xu Yang wrote:
> From: Xu Yang <xu.yang_2@nxp.com>
>
> 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
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));

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@vger.kernel.org
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>   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)