* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-20 7:39 ` neil.armstrong
0 siblings, 0 replies; 32+ messages in thread
From: neil.armstrong @ 2025-05-20 7:39 UTC (permalink / raw)
To: Pritam Manohar Sutar, vkoul, kishon, robh, krzk+dt, conor+dt,
alim.akhtar, andre.draszik, peter.griffin, kauschluss,
m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 16/05/2025 12:26, Pritam Manohar Sutar wrote:
> This SoC has a single USB 3.1 DRD combo phy and three USB2.0
> DRD HS phy controllers those only support the UTMI+ interface.
>
> Support only UTMI+ for this SoC which is very similar to what
> the existing Exynos850 supports.
>
> The combo phy supports both UTMI+ (HS) and PIPE3 (SS) and is
> out of scope of this commit.
>
> Add required change in phy driver to support HS phy for this SoC.
>
> Signed-off-by: Pritam Manohar Sutar <pritam.sutar@samsung.com>
> ---
> drivers/phy/samsung/phy-exynos5-usbdrd.c | 85 ++++++++++++++++++++++++
> 1 file changed, 85 insertions(+)
>
> 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.
> +
> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> + /* utmi init for exynosautov920 HS phy */
> + exynos850_usbdrd_utmi_init(phy_drd);
> +}
> +
> +static int exynosautov920_usbdrd_phy_init(struct phy *phy)
> +{
> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> + int ret = 0;
> +
> + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd->clks);
> + if (ret)
> + return ret;
> +
> + /* UTMI or PIPE3 specific init */
> + inst->phy_cfg->phy_init(phy_drd);
> +
> + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
> +
> + return 0;
> +}
> +
> +static void exynosautov920_v3p1_phy_dis(struct phy *phy)
> +{
> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> + void __iomem *reg_phy = phy_drd->reg_phy;
> + u32 version;
> +
> + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
> +
> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> + exynos850_usbdrd_phy_exit(phy);
> +}
> +
> +static int exynosautov920_usbdrd_phy_exit(struct phy *phy)
> +{
> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> +
> + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
> + exynosautov920_v3p1_phy_dis(phy);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
> + .init = exynosautov920_usbdrd_phy_init,
> + .exit = exynosautov920_usbdrd_phy_exit,
<snip>
> + .id = EXYNOS5_DRDPHY_UTMI,
> + .phy_init = exynosautov920_usbdrd_utmi_init,
<snip>
> + }, {
> + .compatible = "samsung,exynosautov920-usb31drd-phy",
> + .data = &exynosautov920_usb31drd_phy
All those new ops are only called when matching this compatible, it it really
necessary to check the version ? is there "samsung,exynosautov920-usb31drd-phy" PHYs
with version different from 3 in the wild ?
Neil
> },
> { },
> };
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
2025-05-20 7:39 ` neil.armstrong
@ 2025-05-20 7:42 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-20 7:42 UTC (permalink / raw)
To: Neil Armstrong, Pritam Manohar Sutar, vkoul, kishon, robh,
krzk+dt, conor+dt, alim.akhtar, andre.draszik, peter.griffin,
kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
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.
>
>> +
>> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
>> + /* utmi init for exynosautov920 HS phy */
>> + exynos850_usbdrd_utmi_init(phy_drd);
>> +}
>> +
>> +static int exynosautov920_usbdrd_phy_init(struct phy *phy)
>> +{
>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>> + int ret = 0;
>> +
>> + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd->clks);
>> + if (ret)
>> + return ret;
>> +
>> + /* UTMI or PIPE3 specific init */
>> + inst->phy_cfg->phy_init(phy_drd);
>> +
>> + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
>> +
>> + return 0;
>> +}
>> +
>> +static void exynosautov920_v3p1_phy_dis(struct phy *phy)
>> +{
>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>> + void __iomem *reg_phy = phy_drd->reg_phy;
>> + u32 version;
>> +
>> + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
>> +
>> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
>> + exynos850_usbdrd_phy_exit(phy);
>> +}
>> +
>> +static int exynosautov920_usbdrd_phy_exit(struct phy *phy)
>> +{
>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>> +
>> + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
>> + exynosautov920_v3p1_phy_dis(phy);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
>> + .init = exynosautov920_usbdrd_phy_init,
>> + .exit = exynosautov920_usbdrd_phy_exit,
>
> <snip>
>
>> + .id = EXYNOS5_DRDPHY_UTMI,
>> + .phy_init = exynosautov920_usbdrd_utmi_init,
>
> <snip>
>
>> + }, {
>> + .compatible = "samsung,exynosautov920-usb31drd-phy",
>> + .data = &exynosautov920_usb31drd_phy
>
> All those new ops are only called when matching this compatible, it it really
> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-phy" PHYs
> with version different from 3 in the wild ?
Yeah, this looks like downstream code. Anyway this would need
explanation in the commit msg.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-20 7:42 ` Krzysztof Kozlowski
0 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-20 7:42 UTC (permalink / raw)
To: Neil Armstrong, Pritam Manohar Sutar, vkoul, kishon, robh,
krzk+dt, conor+dt, alim.akhtar, andre.draszik, peter.griffin,
kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
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.
>
>> +
>> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
>> + /* utmi init for exynosautov920 HS phy */
>> + exynos850_usbdrd_utmi_init(phy_drd);
>> +}
>> +
>> +static int exynosautov920_usbdrd_phy_init(struct phy *phy)
>> +{
>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>> + int ret = 0;
>> +
>> + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd->clks);
>> + if (ret)
>> + return ret;
>> +
>> + /* UTMI or PIPE3 specific init */
>> + inst->phy_cfg->phy_init(phy_drd);
>> +
>> + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
>> +
>> + return 0;
>> +}
>> +
>> +static void exynosautov920_v3p1_phy_dis(struct phy *phy)
>> +{
>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>> + void __iomem *reg_phy = phy_drd->reg_phy;
>> + u32 version;
>> +
>> + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
>> +
>> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
>> + exynos850_usbdrd_phy_exit(phy);
>> +}
>> +
>> +static int exynosautov920_usbdrd_phy_exit(struct phy *phy)
>> +{
>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>> +
>> + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
>> + exynosautov920_v3p1_phy_dis(phy);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
>> + .init = exynosautov920_usbdrd_phy_init,
>> + .exit = exynosautov920_usbdrd_phy_exit,
>
> <snip>
>
>> + .id = EXYNOS5_DRDPHY_UTMI,
>> + .phy_init = exynosautov920_usbdrd_utmi_init,
>
> <snip>
>
>> + }, {
>> + .compatible = "samsung,exynosautov920-usb31drd-phy",
>> + .data = &exynosautov920_usb31drd_phy
>
> All those new ops are only called when matching this compatible, it it really
> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-phy" PHYs
> with version different from 3 in the wild ?
Yeah, this looks like downstream code. Anyway this would need
explanation in the commit msg.
Best regards,
Krzysztof
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
2025-05-20 7:42 ` Krzysztof Kozlowski
@ 2025-05-21 7:10 ` Pritam Manohar Sutar
-1 siblings, 0 replies; 32+ messages in thread
From: Pritam Manohar Sutar @ 2025-05-21 7:10 UTC (permalink / raw)
To: 'Krzysztof Kozlowski', 'Neil Armstrong', vkoul,
kishon, robh, krzk+dt, conor+dt, alim.akhtar, andre.draszik,
peter.griffin, kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
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
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.
>
> >
> >> +
> >> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> >> + /* utmi init for exynosautov920 HS phy */
> >> + exynos850_usbdrd_utmi_init(phy_drd);
> >> +}
> >> +
> >> +static int exynosautov920_usbdrd_phy_init(struct phy *phy) {
> >> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> >> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> >> + int ret = 0;
> >> +
> >> + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd-
> >clks);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* UTMI or PIPE3 specific init */
> >> + inst->phy_cfg->phy_init(phy_drd);
> >> +
> >> + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks,
> >> +phy_drd->clks);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void exynosautov920_v3p1_phy_dis(struct phy *phy) {
> >> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> >> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> >> + void __iomem *reg_phy = phy_drd->reg_phy;
> >> + u32 version;
> >> +
> >> + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
> >> +
> >> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> >> + exynos850_usbdrd_phy_exit(phy);
> >> +}
> >> +
> >> +static int exynosautov920_usbdrd_phy_exit(struct phy *phy) {
> >> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> >> +
> >> + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
> >> + exynosautov920_v3p1_phy_dis(phy);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
> >> + .init = exynosautov920_usbdrd_phy_init,
> >> + .exit = exynosautov920_usbdrd_phy_exit,
> >
> > <snip>
> >
> >> + .id = EXYNOS5_DRDPHY_UTMI,
> >> + .phy_init = exynosautov920_usbdrd_utmi_init,
> >
> > <snip>
> >
> >> + }, {
> >> + .compatible = "samsung,exynosautov920-usb31drd-phy",
> >> + .data = &exynosautov920_usb31drd_phy
> >
> > All those new ops are only called when matching this compatible, it it
> > really necessary to check the version ? is there
> > "samsung,exynosautov920-usb31drd-phy" PHYs with version different from 3
> in the wild ?
>
>
> Yeah, this looks like downstream code. Anyway this would need explanation in
> the commit msg.
Commit msg can be updated as mentioned below
" This SoC has a single USB 3.1 DRD combo v400 phy that supports
both UTMI+ (HS) and PIPE3 (SS) and three USB2.0 DRD v303 phy
controllers those only support the UTMI+ (HS) interface.
Support only HS phy in this commit which is very similar to what
the existing Exynos850 supports.
Support combo phy later (soon) and this is out of scope of this commit.
Add required change in phy driver to support HS phy for this SoC."
>
> Best regards,
> Krzysztof
Thank you,
Pritam
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-21 7:10 ` Pritam Manohar Sutar
0 siblings, 0 replies; 32+ messages in thread
From: Pritam Manohar Sutar @ 2025-05-21 7:10 UTC (permalink / raw)
To: 'Krzysztof Kozlowski', 'Neil Armstrong', vkoul,
kishon, robh, krzk+dt, conor+dt, alim.akhtar, andre.draszik,
peter.griffin, kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
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
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.
>
> >
> >> +
> >> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> >> + /* utmi init for exynosautov920 HS phy */
> >> + exynos850_usbdrd_utmi_init(phy_drd);
> >> +}
> >> +
> >> +static int exynosautov920_usbdrd_phy_init(struct phy *phy) {
> >> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> >> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> >> + int ret = 0;
> >> +
> >> + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd-
> >clks);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* UTMI or PIPE3 specific init */
> >> + inst->phy_cfg->phy_init(phy_drd);
> >> +
> >> + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks,
> >> +phy_drd->clks);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void exynosautov920_v3p1_phy_dis(struct phy *phy) {
> >> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> >> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> >> + void __iomem *reg_phy = phy_drd->reg_phy;
> >> + u32 version;
> >> +
> >> + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
> >> +
> >> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> >> + exynos850_usbdrd_phy_exit(phy);
> >> +}
> >> +
> >> +static int exynosautov920_usbdrd_phy_exit(struct phy *phy) {
> >> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> >> +
> >> + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
> >> + exynosautov920_v3p1_phy_dis(phy);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
> >> + .init = exynosautov920_usbdrd_phy_init,
> >> + .exit = exynosautov920_usbdrd_phy_exit,
> >
> > <snip>
> >
> >> + .id = EXYNOS5_DRDPHY_UTMI,
> >> + .phy_init = exynosautov920_usbdrd_utmi_init,
> >
> > <snip>
> >
> >> + }, {
> >> + .compatible = "samsung,exynosautov920-usb31drd-phy",
> >> + .data = &exynosautov920_usb31drd_phy
> >
> > All those new ops are only called when matching this compatible, it it
> > really necessary to check the version ? is there
> > "samsung,exynosautov920-usb31drd-phy" PHYs with version different from 3
> in the wild ?
>
>
> Yeah, this looks like downstream code. Anyway this would need explanation in
> the commit msg.
Commit msg can be updated as mentioned below
" This SoC has a single USB 3.1 DRD combo v400 phy that supports
both UTMI+ (HS) and PIPE3 (SS) and three USB2.0 DRD v303 phy
controllers those only support the UTMI+ (HS) interface.
Support only HS phy in this commit which is very similar to what
the existing Exynos850 supports.
Support combo phy later (soon) and this is out of scope of this commit.
Add required change in phy driver to support HS phy for this SoC."
>
> Best regards,
> Krzysztof
Thank you,
Pritam
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
2025-05-21 7:10 ` Pritam Manohar Sutar
@ 2025-05-21 8:47 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-21 8:47 UTC (permalink / raw)
To: Pritam Manohar Sutar, 'Neil Armstrong', vkoul, kishon,
robh, krzk+dt, conor+dt, alim.akhtar, andre.draszik,
peter.griffin, kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-21 8:47 ` Krzysztof Kozlowski
0 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-21 8:47 UTC (permalink / raw)
To: Pritam Manohar Sutar, 'Neil Armstrong', vkoul, kishon,
robh, krzk+dt, conor+dt, alim.akhtar, andre.draszik,
peter.griffin, kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
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
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
2025-05-21 8:47 ` Krzysztof Kozlowski
@ 2025-05-21 8:58 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-21 8:58 UTC (permalink / raw)
To: Pritam Manohar Sutar, 'Neil Armstrong', vkoul, kishon,
robh, krzk+dt, conor+dt, alim.akhtar, andre.draszik,
peter.griffin, kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 21/05/2025 10:47, Krzysztof Kozlowski wrote:
> 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.
Hm, unless you want to say devices are fully compatible, the version is
fully detectable and the driver will make use of that. That would be
fine, but should be expressed in commit msg and actually we should see
the second phy variant already in the patchset.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-21 8:58 ` Krzysztof Kozlowski
0 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-21 8:58 UTC (permalink / raw)
To: Pritam Manohar Sutar, 'Neil Armstrong', vkoul, kishon,
robh, krzk+dt, conor+dt, alim.akhtar, andre.draszik,
peter.griffin, kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 21/05/2025 10:47, Krzysztof Kozlowski wrote:
> 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.
Hm, unless you want to say devices are fully compatible, the version is
fully detectable and the driver will make use of that. That would be
fine, but should be expressed in commit msg and actually we should see
the second phy variant already in the patchset.
Best regards,
Krzysztof
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
2025-05-20 7:39 ` neil.armstrong
@ 2025-05-21 6:56 ` Pritam Manohar Sutar
-1 siblings, 0 replies; 32+ messages in thread
From: Pritam Manohar Sutar @ 2025-05-21 6:56 UTC (permalink / raw)
To: 'Neil Armstrong', vkoul, kishon, robh, krzk+dt, conor+dt,
alim.akhtar, andre.draszik, peter.griffin, kauschluss,
m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
Hi Neil,
Thank you for reviewing the patches.
> -----Original Message-----
> From: neil.armstrong@linaro.org <neil.armstrong@linaro.org>
> Sent: 20 May 2025 01:10 PM
> To: 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 16/05/2025 12:26, Pritam Manohar Sutar wrote:
> > This SoC has a single USB 3.1 DRD combo phy and three USB2.0 DRD HS
> > phy controllers those only support the UTMI+ interface.
> >
> > Support only UTMI+ for this SoC which is very similar to what the
> > existing Exynos850 supports.
> >
> > The combo phy supports both UTMI+ (HS) and PIPE3 (SS) and is out of
> > scope of this commit.
> >
> > Add required change in phy driver to support HS phy for this SoC.
> >
> > Signed-off-by: Pritam Manohar Sutar <pritam.sutar@samsung.com>
> > ---
> > drivers/phy/samsung/phy-exynos5-usbdrd.c | 85
> ++++++++++++++++++++++++
> > 1 file changed, 85 insertions(+)
> >
> > 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.
Will replace dev_info by dev_dbg.
>
> > +
> > + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> > + /* utmi init for exynosautov920 HS phy */
> > + exynos850_usbdrd_utmi_init(phy_drd);
> > +}
> > +
> > +static int exynosautov920_usbdrd_phy_init(struct phy *phy) {
> > + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> > + int ret = 0;
> > +
> > + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd-
> >clks);
> > + if (ret)
> > + return ret;
> > +
> > + /* UTMI or PIPE3 specific init */
> > + inst->phy_cfg->phy_init(phy_drd);
> > +
> > + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks,
> > +phy_drd->clks);
> > +
> > + return 0;
> > +}
> > +
> > +static void exynosautov920_v3p1_phy_dis(struct phy *phy) {
> > + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> > + void __iomem *reg_phy = phy_drd->reg_phy;
> > + u32 version;
> > +
> > + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
> > +
> > + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> > + exynos850_usbdrd_phy_exit(phy);
> > +}
> > +
> > +static int exynosautov920_usbdrd_phy_exit(struct phy *phy) {
> > + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > +
> > + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
> > + exynosautov920_v3p1_phy_dis(phy);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
> > + .init = exynosautov920_usbdrd_phy_init,
> > + .exit = exynosautov920_usbdrd_phy_exit,
>
> <snip>
>
> > + .id = EXYNOS5_DRDPHY_UTMI,
> > + .phy_init = exynosautov920_usbdrd_utmi_init,
>
> <snip>
>
> > + }, {
> > + .compatible = "samsung,exynosautov920-usb31drd-phy",
> > + .data = &exynosautov920_usb31drd_phy
>
> All those new ops are only called when matching this compatible, it it really
> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
> phy" PHYs with version different from 3 in the wild ?
>
This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently,
supporting only v303 phys in this patch-set, and planning v400 phy later (soon).
Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.
> Neil
>
> > },
> > { },
> > };
Thank you,
Pritam
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-21 6:56 ` Pritam Manohar Sutar
0 siblings, 0 replies; 32+ messages in thread
From: Pritam Manohar Sutar @ 2025-05-21 6:56 UTC (permalink / raw)
To: 'Neil Armstrong', vkoul, kishon, robh, krzk+dt, conor+dt,
alim.akhtar, andre.draszik, peter.griffin, kauschluss,
m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
Hi Neil,
Thank you for reviewing the patches.
> -----Original Message-----
> From: neil.armstrong@linaro.org <neil.armstrong@linaro.org>
> Sent: 20 May 2025 01:10 PM
> To: 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 16/05/2025 12:26, Pritam Manohar Sutar wrote:
> > This SoC has a single USB 3.1 DRD combo phy and three USB2.0 DRD HS
> > phy controllers those only support the UTMI+ interface.
> >
> > Support only UTMI+ for this SoC which is very similar to what the
> > existing Exynos850 supports.
> >
> > The combo phy supports both UTMI+ (HS) and PIPE3 (SS) and is out of
> > scope of this commit.
> >
> > Add required change in phy driver to support HS phy for this SoC.
> >
> > Signed-off-by: Pritam Manohar Sutar <pritam.sutar@samsung.com>
> > ---
> > drivers/phy/samsung/phy-exynos5-usbdrd.c | 85
> ++++++++++++++++++++++++
> > 1 file changed, 85 insertions(+)
> >
> > 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.
Will replace dev_info by dev_dbg.
>
> > +
> > + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> > + /* utmi init for exynosautov920 HS phy */
> > + exynos850_usbdrd_utmi_init(phy_drd);
> > +}
> > +
> > +static int exynosautov920_usbdrd_phy_init(struct phy *phy) {
> > + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> > + int ret = 0;
> > +
> > + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd-
> >clks);
> > + if (ret)
> > + return ret;
> > +
> > + /* UTMI or PIPE3 specific init */
> > + inst->phy_cfg->phy_init(phy_drd);
> > +
> > + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks,
> > +phy_drd->clks);
> > +
> > + return 0;
> > +}
> > +
> > +static void exynosautov920_v3p1_phy_dis(struct phy *phy) {
> > + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
> > + void __iomem *reg_phy = phy_drd->reg_phy;
> > + u32 version;
> > +
> > + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
> > +
> > + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
> > + exynos850_usbdrd_phy_exit(phy);
> > +}
> > +
> > +static int exynosautov920_usbdrd_phy_exit(struct phy *phy) {
> > + struct phy_usb_instance *inst = phy_get_drvdata(phy);
> > +
> > + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
> > + exynosautov920_v3p1_phy_dis(phy);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
> > + .init = exynosautov920_usbdrd_phy_init,
> > + .exit = exynosautov920_usbdrd_phy_exit,
>
> <snip>
>
> > + .id = EXYNOS5_DRDPHY_UTMI,
> > + .phy_init = exynosautov920_usbdrd_utmi_init,
>
> <snip>
>
> > + }, {
> > + .compatible = "samsung,exynosautov920-usb31drd-phy",
> > + .data = &exynosautov920_usb31drd_phy
>
> All those new ops are only called when matching this compatible, it it really
> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
> phy" PHYs with version different from 3 in the wild ?
>
This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently,
supporting only v303 phys in this patch-set, and planning v400 phy later (soon).
Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.
> Neil
>
> > },
> > { },
> > };
Thank you,
Pritam
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
2025-05-21 6:56 ` Pritam Manohar Sutar
@ 2025-05-21 12:56 ` 'Neil Armstrong'
-1 siblings, 0 replies; 32+ messages in thread
From: 'Neil Armstrong' @ 2025-05-21 12:56 UTC (permalink / raw)
To: Pritam Manohar Sutar, vkoul, kishon, robh, krzk+dt, conor+dt,
alim.akhtar, andre.draszik, peter.griffin, kauschluss,
m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 21/05/2025 08:56, Pritam Manohar Sutar wrote:
> Hi Neil,
>
> Thank you for reviewing the patches.
>
>> -----Original Message-----
>> From: neil.armstrong@linaro.org <neil.armstrong@linaro.org>
>> Sent: 20 May 2025 01:10 PM
>> To: 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 16/05/2025 12:26, Pritam Manohar Sutar wrote:
>>> This SoC has a single USB 3.1 DRD combo phy and three USB2.0 DRD HS
>>> phy controllers those only support the UTMI+ interface.
>>>
>>> Support only UTMI+ for this SoC which is very similar to what the
>>> existing Exynos850 supports.
>>>
>>> The combo phy supports both UTMI+ (HS) and PIPE3 (SS) and is out of
>>> scope of this commit.
>>>
>>> Add required change in phy driver to support HS phy for this SoC.
>>>
>>> Signed-off-by: Pritam Manohar Sutar <pritam.sutar@samsung.com>
>>> ---
>>> drivers/phy/samsung/phy-exynos5-usbdrd.c | 85
>> ++++++++++++++++++++++++
>>> 1 file changed, 85 insertions(+)
>>>
>>> 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.
>
> Will replace dev_info by dev_dbg.
>
>>
>>> +
>>> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
>>> + /* utmi init for exynosautov920 HS phy */
>>> + exynos850_usbdrd_utmi_init(phy_drd);
>>> +}
>>> +
>>> +static int exynosautov920_usbdrd_phy_init(struct phy *phy) {
>>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>>> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>>> + int ret = 0;
>>> +
>>> + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd-
>>> clks);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* UTMI or PIPE3 specific init */
>>> + inst->phy_cfg->phy_init(phy_drd);
>>> +
>>> + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks,
>>> +phy_drd->clks);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void exynosautov920_v3p1_phy_dis(struct phy *phy) {
>>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>>> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>>> + void __iomem *reg_phy = phy_drd->reg_phy;
>>> + u32 version;
>>> +
>>> + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
>>> +
>>> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
>>> + exynos850_usbdrd_phy_exit(phy);
>>> +}
>>> +
>>> +static int exynosautov920_usbdrd_phy_exit(struct phy *phy) {
>>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>>> +
>>> + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
>>> + exynosautov920_v3p1_phy_dis(phy);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
>>> + .init = exynosautov920_usbdrd_phy_init,
>>> + .exit = exynosautov920_usbdrd_phy_exit,
>>
>> <snip>
>>
>>> + .id = EXYNOS5_DRDPHY_UTMI,
>>> + .phy_init = exynosautov920_usbdrd_utmi_init,
>>
>> <snip>
>>
>>> + }, {
>>> + .compatible = "samsung,exynosautov920-usb31drd-phy",
>>> + .data = &exynosautov920_usb31drd_phy
>>
>> All those new ops are only called when matching this compatible, it it really
>> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
>> phy" PHYs with version different from 3 in the wild ?
>>
>
> This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
> DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently,
> supporting only v303 phys in this patch-set, and planning v400 phy later (soon).
>
> Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.
OK so add 2 compatibles, one for the usb31drd and one for the usb2drd since those are 2 difference hardware.
Neil
>
>> Neil
>>
>>> },
>>> { },
>>> };
>
>
> Thank you,
> Pritam
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-21 12:56 ` 'Neil Armstrong'
0 siblings, 0 replies; 32+ messages in thread
From: 'Neil Armstrong' @ 2025-05-21 12:56 UTC (permalink / raw)
To: Pritam Manohar Sutar, vkoul, kishon, robh, krzk+dt, conor+dt,
alim.akhtar, andre.draszik, peter.griffin, kauschluss,
m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 21/05/2025 08:56, Pritam Manohar Sutar wrote:
> Hi Neil,
>
> Thank you for reviewing the patches.
>
>> -----Original Message-----
>> From: neil.armstrong@linaro.org <neil.armstrong@linaro.org>
>> Sent: 20 May 2025 01:10 PM
>> To: 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 16/05/2025 12:26, Pritam Manohar Sutar wrote:
>>> This SoC has a single USB 3.1 DRD combo phy and three USB2.0 DRD HS
>>> phy controllers those only support the UTMI+ interface.
>>>
>>> Support only UTMI+ for this SoC which is very similar to what the
>>> existing Exynos850 supports.
>>>
>>> The combo phy supports both UTMI+ (HS) and PIPE3 (SS) and is out of
>>> scope of this commit.
>>>
>>> Add required change in phy driver to support HS phy for this SoC.
>>>
>>> Signed-off-by: Pritam Manohar Sutar <pritam.sutar@samsung.com>
>>> ---
>>> drivers/phy/samsung/phy-exynos5-usbdrd.c | 85
>> ++++++++++++++++++++++++
>>> 1 file changed, 85 insertions(+)
>>>
>>> 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.
>
> Will replace dev_info by dev_dbg.
>
>>
>>> +
>>> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
>>> + /* utmi init for exynosautov920 HS phy */
>>> + exynos850_usbdrd_utmi_init(phy_drd);
>>> +}
>>> +
>>> +static int exynosautov920_usbdrd_phy_init(struct phy *phy) {
>>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>>> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>>> + int ret = 0;
>>> +
>>> + ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd-
>>> clks);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* UTMI or PIPE3 specific init */
>>> + inst->phy_cfg->phy_init(phy_drd);
>>> +
>>> + clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks,
>>> +phy_drd->clks);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void exynosautov920_v3p1_phy_dis(struct phy *phy) {
>>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>>> + struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
>>> + void __iomem *reg_phy = phy_drd->reg_phy;
>>> + u32 version;
>>> +
>>> + version = readl(reg_phy + EXYNOSAUTOv920_DRD_CTRL_VER);
>>> +
>>> + if (FIELD_GET(CTRL_VER_MAJOR_VERSION, version) == 0x3)
>>> + exynos850_usbdrd_phy_exit(phy);
>>> +}
>>> +
>>> +static int exynosautov920_usbdrd_phy_exit(struct phy *phy) {
>>> + struct phy_usb_instance *inst = phy_get_drvdata(phy);
>>> +
>>> + if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
>>> + exynosautov920_v3p1_phy_dis(phy);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct phy_ops exynosautov920_usbdrd_phy_ops = {
>>> + .init = exynosautov920_usbdrd_phy_init,
>>> + .exit = exynosautov920_usbdrd_phy_exit,
>>
>> <snip>
>>
>>> + .id = EXYNOS5_DRDPHY_UTMI,
>>> + .phy_init = exynosautov920_usbdrd_utmi_init,
>>
>> <snip>
>>
>>> + }, {
>>> + .compatible = "samsung,exynosautov920-usb31drd-phy",
>>> + .data = &exynosautov920_usb31drd_phy
>>
>> All those new ops are only called when matching this compatible, it it really
>> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
>> phy" PHYs with version different from 3 in the wild ?
>>
>
> This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
> DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently,
> supporting only v303 phys in this patch-set, and planning v400 phy later (soon).
>
> Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.
OK so add 2 compatibles, one for the usb31drd and one for the usb2drd since those are 2 difference hardware.
Neil
>
>> Neil
>>
>>> },
>>> { },
>>> };
>
>
> Thank you,
> Pritam
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
2025-05-21 12:56 ` 'Neil Armstrong'
@ 2025-05-21 12:58 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-21 12:58 UTC (permalink / raw)
To: Neil Armstrong, Pritam Manohar Sutar, vkoul, kishon, robh,
krzk+dt, conor+dt, alim.akhtar, andre.draszik, peter.griffin,
kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 21/05/2025 14:56, 'Neil Armstrong' wrote:
>>> <snip>
>>>
>>>> + }, {
>>>> + .compatible = "samsung,exynosautov920-usb31drd-phy",
>>>> + .data = &exynosautov920_usb31drd_phy
>>>
>>> All those new ops are only called when matching this compatible, it it really
>>> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
>>> phy" PHYs with version different from 3 in the wild ?
>>>
>>
>> This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
>> DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently,
>> supporting only v303 phys in this patch-set, and planning v400 phy later (soon).
>>
>> Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.
>
> OK so add 2 compatibles, one for the usb31drd and one for the usb2drd since those are 2 difference hardware.
...unless this is fully detectable hardware, then use fallbacks and only
one compatible in the driver.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-21 12:58 ` Krzysztof Kozlowski
0 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2025-05-21 12:58 UTC (permalink / raw)
To: Neil Armstrong, Pritam Manohar Sutar, vkoul, kishon, robh,
krzk+dt, conor+dt, alim.akhtar, andre.draszik, peter.griffin,
kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 21/05/2025 14:56, 'Neil Armstrong' wrote:
>>> <snip>
>>>
>>>> + }, {
>>>> + .compatible = "samsung,exynosautov920-usb31drd-phy",
>>>> + .data = &exynosautov920_usb31drd_phy
>>>
>>> All those new ops are only called when matching this compatible, it it really
>>> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
>>> phy" PHYs with version different from 3 in the wild ?
>>>
>>
>> This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
>> DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently,
>> supporting only v303 phys in this patch-set, and planning v400 phy later (soon).
>>
>> Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.
>
> OK so add 2 compatibles, one for the usb31drd and one for the usb2drd since those are 2 difference hardware.
...unless this is fully detectable hardware, then use fallbacks and only
one compatible in the driver.
Best regards,
Krzysztof
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
2025-05-21 12:58 ` Krzysztof Kozlowski
@ 2025-05-22 7:22 ` Neil Armstrong
-1 siblings, 0 replies; 32+ messages in thread
From: Neil Armstrong @ 2025-05-22 7:22 UTC (permalink / raw)
To: Krzysztof Kozlowski, Pritam Manohar Sutar, vkoul, kishon, robh,
krzk+dt, conor+dt, alim.akhtar, andre.draszik, peter.griffin,
kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 21/05/2025 14:58, Krzysztof Kozlowski wrote:
> On 21/05/2025 14:56, 'Neil Armstrong' wrote:
>>>> <snip>
>>>>
>>>>> + }, {
>>>>> + .compatible = "samsung,exynosautov920-usb31drd-phy",
>>>>> + .data = &exynosautov920_usb31drd_phy
>>>>
>>>> All those new ops are only called when matching this compatible, it it really
>>>> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
>>>> phy" PHYs with version different from 3 in the wild ?
>>>>
>>>
>>> This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
>>> DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently,
>>> supporting only v303 phys in this patch-set, and planning v400 phy later (soon).
>>>
>>> Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.
>>
>> OK so add 2 compatibles, one for the usb31drd and one for the usb2drd since those are 2 difference hardware.
>
> ...unless this is fully detectable hardware, then use fallbacks and only
> one compatible in the driver.
But use proper compatible like "samsung,exynosautov92-usb-phy", but still those are
2 very different PHYs connected to different HW blocks, I won't mix USB3.1 and USB2 PHY
compatibles.
Neil
>
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v2 2/2] phy: exyons5-usbdrd: support HS phy for ExynosAutov920
@ 2025-05-22 7:22 ` Neil Armstrong
0 siblings, 0 replies; 32+ messages in thread
From: Neil Armstrong @ 2025-05-22 7:22 UTC (permalink / raw)
To: Krzysztof Kozlowski, Pritam Manohar Sutar, vkoul, kishon, robh,
krzk+dt, conor+dt, alim.akhtar, andre.draszik, peter.griffin,
kauschluss, m.szyprowski, s.nawrocki
Cc: linux-phy, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, rosa.pila, dev.tailor, faraz.ata, muhammed.ali,
selvarasu.g
On 21/05/2025 14:58, Krzysztof Kozlowski wrote:
> On 21/05/2025 14:56, 'Neil Armstrong' wrote:
>>>> <snip>
>>>>
>>>>> + }, {
>>>>> + .compatible = "samsung,exynosautov920-usb31drd-phy",
>>>>> + .data = &exynosautov920_usb31drd_phy
>>>>
>>>> All those new ops are only called when matching this compatible, it it really
>>>> necessary to check the version ? is there "samsung,exynosautov920-usb31drd-
>>>> phy" PHYs with version different from 3 in the wild ?
>>>>
>>>
>>> This SoC has a single USB 3.1 DRD combo phy of version v400 (major : minor versions) and three USB2.0
>>> DRD phy v303 (major : minor versions) controllers those only support the UTMI+ interface. Currently,
>>> supporting only v303 phys in this patch-set, and planning v400 phy later (soon).
>>>
>>> Yes, there's v400 phy version that is different from v303 phy. Hence, phy version check is needed to support both the phys for same compatible.
>>
>> OK so add 2 compatibles, one for the usb31drd and one for the usb2drd since those are 2 difference hardware.
>
> ...unless this is fully detectable hardware, then use fallbacks and only
> one compatible in the driver.
But use proper compatible like "samsung,exynosautov92-usb-phy", but still those are
2 very different PHYs connected to different HW blocks, I won't mix USB3.1 and USB2 PHY
compatibles.
Neil
>
> Best regards,
> Krzysztof
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 32+ messages in thread