Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920

[Krzysztof Kozlowski <krzk@kernel.org>]

On 21/05/2025 09:10, Pritam Manohar Sutar wrote:
> Hi Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: 20 May 2025 01:13 PM
>> To: Neil Armstrong <neil.armstrong@linaro.org>; Pritam Manohar Sutar
>> <pritam.sutar@samsung.com>; vkoul@kernel.org; kishon@kernel.org;
>> robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
>> alim.akhtar@samsung.com; andre.draszik@linaro.org; peter.griffin@linaro.org;
>> kauschluss@disroot.org; m.szyprowski@samsung.com;
>> s.nawrocki@samsung.com
>> Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
>> soc@vger.kernel.org; rosa.pila@samsung.com; dev.tailor@samsung.com;
>> faraz.ata@samsung.com; muhammed.ali@samsung.com;
>> selvarasu.g@samsung.com
>> Subject: Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for
>> ExynosAutov920
>>
>> On 20/05/2025 09:39, neil.armstrong@linaro.org wrote:
>>>> diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c
>>>> b/drivers/phy/samsung/phy-exynos5-usbdrd.c
>>>> index 634c4310c660..b440b56c6595 100644
>>>> --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
>>>> +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
>>>> @@ -177,6 +177,9 @@
>>>>   #define HSPHYPLLTUNE_PLL_P_TUNE			GENMASK(3,
>> 0)
>>>>
>>>>   /* Exynos850: USB DRD PHY registers */
>>>> +#define EXYNOSAUTOv920_DRD_CTRL_VER		0x00
>>>> +#define CTRL_VER_MAJOR_VERSION			GENMASK(31,
>> 24)
>>>> +
>>>>   #define EXYNOS850_DRD_LINKCTRL			0x04
>>>>   #define LINKCTRL_FORCE_RXELECIDLE		BIT(18)
>>>>   #define LINKCTRL_FORCE_PHYSTATUS		BIT(17)
>>>> @@ -1772,6 +1775,10 @@ static const char * const
>> exynos5_regulator_names[] = {
>>>>   	"vbus", "vbus-boost",
>>>>   };
>>>>
>>>> +static const char * const exynosautov920_clk_names[] = {
>>>> +	"ext_xtal",
>>>> +};
>>>> +
>>>>   static const struct exynos5_usbdrd_phy_drvdata exynos5420_usbdrd_phy =
>> {
>>>>   	.phy_cfg		= phy_cfg_exynos5,
>>>>   	.phy_ops		= &exynos5_usbdrd_phy_ops,
>>>> @@ -1847,6 +1854,81 @@ static const struct exynos5_usbdrd_phy_drvdata
>> exynos850_usbdrd_phy = {
>>>>   	.n_regulators		= ARRAY_SIZE(exynos5_regulator_names),
>>>>   };
>>>>
>>>> +static void exynosautov920_usbdrd_utmi_init(struct
>>>> +exynos5_usbdrd_phy *phy_drd) {
>>>> +	u32 version;
>>>> +
>>>> +	version = readl(phy_drd->reg_phy +
>> EXYNOSAUTOv920_DRD_CTRL_VER);
>>>> +	dev_info(phy_drd->dev, "usbphy: version:0x%x\n", version);
>>>
>>> Please do not add mode info to boot log, use dev_dbg instead.
>>
>> Just drop entirely, not even worth dbg (see coding style, driver development
>> debugging guide). It is fixed per given compatible, isn't it? If not, there is entire
>> commit msg to explain unusual things.
> 
> This SoC has a single USB 3.1 DRD combo v400 phy and three USB2.0 DRD phy v303


That's a different device, no? Look at the compatible here - it says
usb31drd.

What does 31 stand for?

> controllers those only support the UTMI+ interface. Currently, supporting only 
> v303 phy in this patch-set, and planning v400 phy later (soon). Same may be 
> also updated in commit  message. 
> 
> If there's any issue in phy init, dbg print is needed to debug which phy caused it. 
No, rethink rather this makes sense at all. Please read carefully
writing bindings, which will tell you that you cannot have different
devices under the same compatible. Unless you say these are the same
devices and it differs by other phy? But this is a phy... so many questions.

Best regards,
Krzysztof