Message ID | 20250223122227.725233-1-ivo.ivanov.ivanov1@gmail.com |
---|---|
Headers | show |
Series | phy: samsung: add Exynos2200 SNPS eUSB2 driver | expand |
On Sun, 23 Feb 2025 14:22:22 +0200, Ivaylo Ivanov wrote: > The Exynos2200 SoC has a USB controller PHY, which acts as an > intermediary between a USB controller (typically DWC3) and other PHYs > (UTMI, PIPE3). Add a dt-binding schema for it. > > Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> > --- > .../phy/samsung,exynos2200-usbcon-phy.yaml | 76 +++++++++++++++++++ > 1 file changed, 76 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.example.dts:18:18: fatal error: dt-bindings/clock/samsung,exynos2200-cmu.h: No such file or directory 18 | #include <dt-bindings/clock/samsung,exynos2200-cmu.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1511: dt_binding_check] Error 2 make: *** [Makefile:251: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250223122227.725233-4-ivo.ivanov.ivanov1@gmail.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Sun, Feb 23, 2025 at 02:22:26PM +0200, Ivaylo Ivanov wrote: > The Exynos2200 SoC reuses the Synopsis eUSB2 PHY IP, alongside an > external repeater, for USB 2.0. Add support for it to the existing > driver. > > Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> > --- > drivers/phy/Kconfig | 2 +- > drivers/phy/phy-snps-eusb2.c | 172 +++++++++++++++++++++++++++++++++++ > 2 files changed, 173 insertions(+), 1 deletion(-) > > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig > index 11c166204..58c911e1b 100644 > --- a/drivers/phy/Kconfig > +++ b/drivers/phy/Kconfig > @@ -45,7 +45,7 @@ config PHY_PISTACHIO_USB > > config PHY_SNPS_EUSB2 > tristate "SNPS eUSB2 PHY Driver" > - depends on OF && (ARCH_QCOM || COMPILE_TEST) > + depends on OF && (ARCH_EXYNOS || ARCH_QCOM || COMPILE_TEST) > select GENERIC_PHY > help > Enable support for the USB high-speed SNPS eUSB2 phy on select > diff --git a/drivers/phy/phy-snps-eusb2.c b/drivers/phy/phy-snps-eusb2.c > index 7a242fe32..67a19d671 100644 > --- a/drivers/phy/phy-snps-eusb2.c > +++ b/drivers/phy/phy-snps-eusb2.c > @@ -13,6 +13,39 @@ > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > > +#define EXYNOS_USB_PHY_HS_PHY_CTRL_RST (0x0) > +#define USB_PHY_RST_MASK GENMASK(1, 0) > +#define UTMI_PORT_RST_MASK GENMASK(5, 4) > + > +#define EXYNOS_USB_PHY_HS_PHY_CTRL_COMMON (0x4) > +#define RPTR_MODE BIT(10) > +#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 EXYNOS_USB_PHY_CFG_PLLCFG0 (0x8) > +#define PHY_CFG_PLL_FB_DIV_19_8_MASK GENMASK(19, 8) > +#define DIV_19_8_19_2_MHZ_VAL (0x170) > +#define DIV_19_8_20_MHZ_VAL (0x160) > +#define DIV_19_8_24_MHZ_VAL (0x120) > +#define DIV_19_8_26_MHZ_VAL (0x107) > +#define DIV_19_8_48_MHZ_VAL (0x120) > + > +#define EXYNOS_USB_PHY_CFG_PLLCFG1 (0xc) > +#define EXYNOS_PHY_CFG_PLL_FB_DIV_11_8_MASK GENMASK(11, 8) > +#define EXYNOS_DIV_11_8_19_2_MHZ_VAL (0x0) > +#define EXYNOS_DIV_11_8_20_MHZ_VAL (0x0) > +#define EXYNOS_DIV_11_8_24_MHZ_VAL (0x0) > +#define EXYNOS_DIV_11_8_26_MHZ_VAL (0x0) > +#define EXYNOS_DIV_11_8_48_MHZ_VAL (0x1) > + > +#define EXYNOS_PHY_CFG_TX (0x14) > +#define EXYNOS_PHY_CFG_TX_FSLS_VREF_TUNE_MASK GENMASK(2, 1) > + > +#define EXYNOS_USB_PHY_UTMI_TESTSE (0x20) > +#define TEST_IDDQ BIT(6) > + > #define QCOM_USB_PHY_UTMI_CTRL0 (0x3c) > #define SLEEPM BIT(0) > #define OPMODE_MASK GENMASK(4, 3) > @@ -196,6 +229,93 @@ static void qcom_eusb2_default_parameters(struct snps_eusb2_hsphy *phy) > FIELD_PREP(PHY_CFG_TX_HS_XV_TUNE_MASK, 0x0)); > } > > +static int exynos_eusb2_ref_clk_init(struct snps_eusb2_hsphy *phy) > +{ > + unsigned long ref_clk_freq = clk_get_rate(phy->ref_clk); > + > + switch (ref_clk_freq) { > + case 19200000: > + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_HS_PHY_CTRL_COMMON, > + FSEL_MASK, > + FIELD_PREP(FSEL_MASK, FSEL_19_2_MHZ_VAL)); > + Could you please unify the switchcase? assign the values to temp variables, then program them from a single code stream. Or maybe even replace switch-case with a table-based lookup. (we probably should implement the similar change for qcom part. Maybe you can refactor it too?) Other than that LGTM. > + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG0, > + PHY_CFG_PLL_FB_DIV_19_8_MASK, > + FIELD_PREP(PHY_CFG_PLL_FB_DIV_19_8_MASK, > + DIV_19_8_19_2_MHZ_VAL)); > + > + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG1, > + EXYNOS_PHY_CFG_PLL_FB_DIV_11_8_MASK, > + EXYNOS_DIV_11_8_19_2_MHZ_VAL); > + break; > + > + case 20000000: > + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_HS_PHY_CTRL_COMMON, > + FSEL_MASK, > + FIELD_PREP(FSEL_MASK, FSEL_20_MHZ_VAL)); > + > + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG0, > + PHY_CFG_PLL_FB_DIV_19_8_MASK, > + FIELD_PREP(PHY_CFG_PLL_FB_DIV_19_8_MASK, > + DIV_19_8_20_MHZ_VAL)); > + > + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG1, > + EXYNOS_PHY_CFG_PLL_FB_DIV_11_8_MASK, > + EXYNOS_DIV_11_8_20_MHZ_VAL); > + break; > +
On 2/24/25 01:51, Dmitry Baryshkov wrote: > On Sun, Feb 23, 2025 at 02:22:26PM +0200, Ivaylo Ivanov wrote: >> The Exynos2200 SoC reuses the Synopsis eUSB2 PHY IP, alongside an >> external repeater, for USB 2.0. Add support for it to the existing >> driver. >> >> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >> --- >> drivers/phy/Kconfig | 2 +- >> drivers/phy/phy-snps-eusb2.c | 172 +++++++++++++++++++++++++++++++++++ >> 2 files changed, 173 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >> index 11c166204..58c911e1b 100644 >> --- a/drivers/phy/Kconfig >> +++ b/drivers/phy/Kconfig >> @@ -45,7 +45,7 @@ config PHY_PISTACHIO_USB >> >> config PHY_SNPS_EUSB2 >> tristate "SNPS eUSB2 PHY Driver" >> - depends on OF && (ARCH_QCOM || COMPILE_TEST) >> + depends on OF && (ARCH_EXYNOS || ARCH_QCOM || COMPILE_TEST) >> select GENERIC_PHY >> help >> Enable support for the USB high-speed SNPS eUSB2 phy on select >> diff --git a/drivers/phy/phy-snps-eusb2.c b/drivers/phy/phy-snps-eusb2.c >> index 7a242fe32..67a19d671 100644 >> --- a/drivers/phy/phy-snps-eusb2.c >> +++ b/drivers/phy/phy-snps-eusb2.c >> @@ -13,6 +13,39 @@ >> #include <linux/regulator/consumer.h> >> #include <linux/reset.h> >> >> +#define EXYNOS_USB_PHY_HS_PHY_CTRL_RST (0x0) >> +#define USB_PHY_RST_MASK GENMASK(1, 0) >> +#define UTMI_PORT_RST_MASK GENMASK(5, 4) >> + >> +#define EXYNOS_USB_PHY_HS_PHY_CTRL_COMMON (0x4) >> +#define RPTR_MODE BIT(10) >> +#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 EXYNOS_USB_PHY_CFG_PLLCFG0 (0x8) >> +#define PHY_CFG_PLL_FB_DIV_19_8_MASK GENMASK(19, 8) >> +#define DIV_19_8_19_2_MHZ_VAL (0x170) >> +#define DIV_19_8_20_MHZ_VAL (0x160) >> +#define DIV_19_8_24_MHZ_VAL (0x120) >> +#define DIV_19_8_26_MHZ_VAL (0x107) >> +#define DIV_19_8_48_MHZ_VAL (0x120) >> + >> +#define EXYNOS_USB_PHY_CFG_PLLCFG1 (0xc) >> +#define EXYNOS_PHY_CFG_PLL_FB_DIV_11_8_MASK GENMASK(11, 8) >> +#define EXYNOS_DIV_11_8_19_2_MHZ_VAL (0x0) >> +#define EXYNOS_DIV_11_8_20_MHZ_VAL (0x0) >> +#define EXYNOS_DIV_11_8_24_MHZ_VAL (0x0) >> +#define EXYNOS_DIV_11_8_26_MHZ_VAL (0x0) >> +#define EXYNOS_DIV_11_8_48_MHZ_VAL (0x1) >> + >> +#define EXYNOS_PHY_CFG_TX (0x14) >> +#define EXYNOS_PHY_CFG_TX_FSLS_VREF_TUNE_MASK GENMASK(2, 1) >> + >> +#define EXYNOS_USB_PHY_UTMI_TESTSE (0x20) >> +#define TEST_IDDQ BIT(6) >> + >> #define QCOM_USB_PHY_UTMI_CTRL0 (0x3c) >> #define SLEEPM BIT(0) >> #define OPMODE_MASK GENMASK(4, 3) >> @@ -196,6 +229,93 @@ static void qcom_eusb2_default_parameters(struct snps_eusb2_hsphy *phy) >> FIELD_PREP(PHY_CFG_TX_HS_XV_TUNE_MASK, 0x0)); >> } >> >> +static int exynos_eusb2_ref_clk_init(struct snps_eusb2_hsphy *phy) >> +{ >> + unsigned long ref_clk_freq = clk_get_rate(phy->ref_clk); >> + >> + switch (ref_clk_freq) { >> + case 19200000: >> + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_HS_PHY_CTRL_COMMON, >> + FSEL_MASK, >> + FIELD_PREP(FSEL_MASK, FSEL_19_2_MHZ_VAL)); >> + > Could you please unify the switchcase? assign the values to temp > variables, then program them from a single code stream. Or maybe even > replace switch-case with a table-based lookup. > > (we probably should implement the similar change for qcom part. Maybe > you can refactor it too?) Alright. I'll do it for the Qualcomm part too in a separate commit. Thanks for the feedback! Best regards, Ivaylo > Other than that LGTM. > >> + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG0, >> + PHY_CFG_PLL_FB_DIV_19_8_MASK, >> + FIELD_PREP(PHY_CFG_PLL_FB_DIV_19_8_MASK, >> + DIV_19_8_19_2_MHZ_VAL)); >> + >> + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG1, >> + EXYNOS_PHY_CFG_PLL_FB_DIV_11_8_MASK, >> + EXYNOS_DIV_11_8_19_2_MHZ_VAL); >> + break; >> + >> + case 20000000: >> + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_HS_PHY_CTRL_COMMON, >> + FSEL_MASK, >> + FIELD_PREP(FSEL_MASK, FSEL_20_MHZ_VAL)); >> + >> + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG0, >> + PHY_CFG_PLL_FB_DIV_19_8_MASK, >> + FIELD_PREP(PHY_CFG_PLL_FB_DIV_19_8_MASK, >> + DIV_19_8_20_MHZ_VAL)); >> + >> + snps_eusb2_hsphy_write_mask(phy->base, EXYNOS_USB_PHY_CFG_PLLCFG1, >> + EXYNOS_PHY_CFG_PLL_FB_DIV_11_8_MASK, >> + EXYNOS_DIV_11_8_20_MHZ_VAL); >> + break; >> +
On Sun, Feb 23, 2025 at 02:22:22PM +0200, Ivaylo Ivanov wrote: > The Exynos2200 SoC has a USB controller PHY, which acts as an > intermediary between a USB controller (typically DWC3) and other PHYs > (UTMI, PIPE3). Add a dt-binding schema for it. > > Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> > --- > .../phy/samsung,exynos2200-usbcon-phy.yaml | 76 +++++++++++++++++++ > 1 file changed, 76 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml You have undocumented dependencies which prevent merging this file. First, dependencies have to be clearly expressed. Second, you should rather decouple the code from header dependencies, otherwise this cannot be merged for current release (just use clocks with long names, without IDs). > > diff --git a/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml > new file mode 100644 > index 000000000..7d879ec8b > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml > @@ -0,0 +1,76 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/samsung,exynos2200-usbcon-phy.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Exynos2200 USB controller PHY > + > +maintainers: > + - Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> > + > +description: > + Exynos2200 USB controller PHY is an intermediary between a USB controller > + (typically DWC3) and other PHYs (UTMI, PIPE3). Isn't this the same as usbdrd phy? see: samsung,usb3-drd-phy.yaml I think there is no PHY between DWC3 and UTMI/PIPE. There is a PHY controller (so the samsung,usb3-drd-phy.yaml) which we call here the phy. Best regards, Krzysztof
On 2/24/25 10:56, Krzysztof Kozlowski wrote: > On Sun, Feb 23, 2025 at 02:22:22PM +0200, Ivaylo Ivanov wrote: >> The Exynos2200 SoC has a USB controller PHY, which acts as an >> intermediary between a USB controller (typically DWC3) and other PHYs >> (UTMI, PIPE3). Add a dt-binding schema for it. >> >> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >> --- >> .../phy/samsung,exynos2200-usbcon-phy.yaml | 76 +++++++++++++++++++ >> 1 file changed, 76 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml > You have undocumented dependencies which prevent merging this file. > First, dependencies have to be clearly expressed. They are, in the cover letter. > Second, you should > rather decouple the code from header dependencies, otherwise this cannot > be merged for current release (just use clocks with long names, without IDs). Sure > >> diff --git a/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >> new file mode 100644 >> index 000000000..7d879ec8b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >> @@ -0,0 +1,76 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/phy/samsung,exynos2200-usbcon-phy.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Exynos2200 USB controller PHY >> + >> +maintainers: >> + - Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >> + >> +description: >> + Exynos2200 USB controller PHY is an intermediary between a USB controller >> + (typically DWC3) and other PHYs (UTMI, PIPE3). > Isn't this the same as usbdrd phy? see: samsung,usb3-drd-phy.yaml It's not (I think). There's a few reasons I've decided to make this separate from the usb3-drd-phy bindings and exynos5-usbdrd driver: 1. This PHY does not provide UTMI and PIPE3 on its own. There's no tuning for them, and all that is needed from it is to disable HWACG, assert/ deassert reset and force bvalid/vbusvalid. After that SNPS eUSB2 initialization can be done and USB2 works. If the USBCON phy is not set up before the eUSB2 one, the device hangs, so there is definitely a dependancy between them. For PIPE3 we'd need to control the pipe3 attaching/deattaching and then initialize the synopsys USBDP combophy. 2. With the way it's modelled, we need to parse phandles from eUSB2 and USBDP to the controller. Adding that to the usbdrd driver would be... weird. It makes more sense to model it as a separate driver, because it functions in a different way. I've described this in the cover letter as well. Best regards, Ivaylo > > I think there is no PHY between DWC3 and UTMI/PIPE. There is a PHY > controller (so the samsung,usb3-drd-phy.yaml) which we call here the > phy. > > > Best regards, > Krzysztof >
Hi, On 23/02/2025 13:22, Ivaylo Ivanov wrote: > As Samsung is using the same Synopsys eUSB2 IP in Exynos2200, albeit > with a different register layout, it only makes sense to implement > support for that in the existing eUSB2 driver. > > To make room for new non-qcom SoCs, do the following: > 1. Move phy-qcom-snps-eusb2.c to phy-snps-eusb2.c > 2. Rename all qcom_snps_eusb2 functions and structs to snps_eusb2_phy > 3. Add a prefix to the qcom-specific register offset definitions > 4. Make a generic phy_ops init that sets up power before the SoC-specific > eUSB2 IP init > 5. Introduce a driver data structure with init function and clocks Please split this in multiples patches, because we can't check the actual changes on the driver... so it's non reviewable in the current state. Neil > > Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> > Suggested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > drivers/phy/Kconfig | 8 + > drivers/phy/Makefile | 1 + > drivers/phy/phy-snps-eusb2.c | 505 +++++++++++++++++++++ > drivers/phy/qualcomm/Kconfig | 9 - > drivers/phy/qualcomm/Makefile | 1 - > drivers/phy/qualcomm/phy-qcom-snps-eusb2.c | 442 ------------------ > 6 files changed, 514 insertions(+), 452 deletions(-) > create mode 100644 drivers/phy/phy-snps-eusb2.c > delete mode 100644 drivers/phy/qualcomm/phy-qcom-snps-eusb2.c > > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig > index 8d58efe99..11c166204 100644 > --- a/drivers/phy/Kconfig > +++ b/drivers/phy/Kconfig > @@ -43,6 +43,14 @@ config PHY_PISTACHIO_USB > help > Enable this to support the USB2.0 PHY on the IMG Pistachio SoC. > > +config PHY_SNPS_EUSB2 > + tristate "SNPS eUSB2 PHY Driver" > + depends on OF && (ARCH_QCOM || COMPILE_TEST) > + select GENERIC_PHY > + help > + Enable support for the USB high-speed SNPS eUSB2 phy on select > + SoCs. The PHY is usually paired with a Synopsys DWC3 USB controller. > + > config PHY_XGENE > tristate "APM X-Gene 15Gbps PHY support" > depends on HAS_IOMEM && OF && (ARCH_XGENE || COMPILE_TEST) > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile > index e281442ac..c670a8dac 100644 > --- a/drivers/phy/Makefile > +++ b/drivers/phy/Makefile > @@ -9,6 +9,7 @@ obj-$(CONFIG_PHY_CAN_TRANSCEIVER) += phy-can-transceiver.o > obj-$(CONFIG_PHY_LPC18XX_USB_OTG) += phy-lpc18xx-usb-otg.o > obj-$(CONFIG_PHY_XGENE) += phy-xgene.o > obj-$(CONFIG_PHY_PISTACHIO_USB) += phy-pistachio-usb.o > +obj-$(CONFIG_PHY_SNPS_EUSB2) += phy-snps-eusb2.o > obj-$(CONFIG_USB_LGM_PHY) += phy-lgm-usb.o > obj-$(CONFIG_PHY_AIROHA_PCIE) += phy-airoha-pcie.o > obj-$(CONFIG_PHY_NXP_PTN3222) += phy-nxp-ptn3222.o > diff --git a/drivers/phy/phy-snps-eusb2.c b/drivers/phy/phy-snps-eusb2.c > new file mode 100644 > index 000000000..4e5914a76 > --- /dev/null > +++ b/drivers/phy/phy-snps-eusb2.c > @@ -0,0 +1,505 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2023, Linaro Limited > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/iopoll.h> > +#include <linux/mod_devicetable.h> > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > +#include <linux/reset.h> > + > +#define QCOM_USB_PHY_UTMI_CTRL0 (0x3c) > +#define SLEEPM BIT(0) > +#define OPMODE_MASK GENMASK(4, 3) > +#define OPMODE_NONDRIVING BIT(3) > + > +#define QCOM_USB_PHY_UTMI_CTRL5 (0x50) > +#define POR BIT(1) > + > +#define QCOM_USB_PHY_HS_PHY_CTRL_COMMON0 (0x54) > +#define PHY_ENABLE BIT(0) > +#define SIDDQ_SEL BIT(1) > +#define SIDDQ BIT(2) > +#define RETENABLEN BIT(3) > +#define FSEL_MASK GENMASK(6, 4) > +#define FSEL_19_2_MHZ_VAL (0x0) > +#define FSEL_38_4_MHZ_VAL (0x4) > + > +#define QCOM_USB_PHY_CFG_CTRL_1 (0x58) > +#define PHY_CFG_PLL_CPBIAS_CNTRL_MASK GENMASK(7, 1) > + > +#define QCOM_USB_PHY_CFG_CTRL_2 (0x5c) > +#define PHY_CFG_PLL_FB_DIV_7_0_MASK GENMASK(7, 0) > +#define DIV_7_0_19_2_MHZ_VAL (0x90) > +#define DIV_7_0_38_4_MHZ_VAL (0xc8) > + > +#define QCOM_USB_PHY_CFG_CTRL_3 (0x60) > +#define PHY_CFG_PLL_FB_DIV_11_8_MASK GENMASK(3, 0) > +#define DIV_11_8_19_2_MHZ_VAL (0x1) > +#define DIV_11_8_38_4_MHZ_VAL (0x0) > + > +#define PHY_CFG_PLL_REF_DIV GENMASK(7, 4) > +#define PLL_REF_DIV_VAL (0x0) > + > +#define QCOM_USB_PHY_HS_PHY_CTRL2 (0x64) > +#define VBUSVLDEXT0 BIT(0) > +#define USB2_SUSPEND_N BIT(2) > +#define USB2_SUSPEND_N_SEL BIT(3) > +#define VBUS_DET_EXT_SEL BIT(4) > + > +#define QCOM_USB_PHY_CFG_CTRL_4 (0x68) > +#define PHY_CFG_PLL_GMP_CNTRL_MASK GENMASK(1, 0) > +#define PHY_CFG_PLL_INT_CNTRL_MASK GENMASK(7, 2) > + > +#define QCOM_USB_PHY_CFG_CTRL_5 (0x6c) > +#define PHY_CFG_PLL_PROP_CNTRL_MASK GENMASK(4, 0) > +#define PHY_CFG_PLL_VREF_TUNE_MASK GENMASK(7, 6) > + > +#define QCOM_USB_PHY_CFG_CTRL_6 (0x70) > +#define PHY_CFG_PLL_VCO_CNTRL_MASK GENMASK(2, 0) > + > +#define QCOM_USB_PHY_CFG_CTRL_7 (0x74) > + > +#define QCOM_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 QCOM_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) > + > +#define QCOM_USB_PHY_CFG_CTRL_10 (0x80) > + > +#define QCOM_USB_PHY_CFG0 (0x94) > +#define DATAPATH_CTRL_OVERRIDE_EN BIT(0) > +#define CMN_CTRL_OVERRIDE_EN BIT(1) > + > +#define QCOM_UTMI_PHY_CMN_CTRL0 (0x98) > +#define TESTBURNIN BIT(6) > + > +#define QCOM_USB_PHY_FSEL_SEL (0xb8) > +#define FSEL_SEL BIT(0) > + > +#define QCOM_USB_PHY_APB_ACCESS_CMD (0x130) > +#define RW_ACCESS BIT(0) > +#define APB_START_CMD BIT(1) > +#define APB_LOGIC_RESET BIT(2) > + > +#define QCOM_USB_PHY_APB_ACCESS_STATUS (0x134) > +#define ACCESS_DONE BIT(0) > +#define TIMED_OUT BIT(1) > +#define ACCESS_ERROR BIT(2) > +#define ACCESS_IN_PROGRESS BIT(3) > + > +#define QCOM_USB_PHY_APB_ADDRESS (0x138) > +#define APB_REG_ADDR_MASK GENMASK(7, 0) > + > +#define QCOM_USB_PHY_APB_WRDATA_LSB (0x13c) > +#define APB_REG_WRDATA_7_0_MASK GENMASK(3, 0) > + > +#define QCOM_USB_PHY_APB_WRDATA_MSB (0x140) > +#define APB_REG_WRDATA_15_8_MASK GENMASK(7, 4) > + > +#define QCOM_USB_PHY_APB_RDDATA_LSB (0x144) > +#define APB_REG_RDDATA_7_0_MASK GENMASK(3, 0) > + > +#define QCOM_USB_PHY_APB_RDDATA_MSB (0x148) > +#define APB_REG_RDDATA_15_8_MASK GENMASK(7, 4) > + > +static const char * const eusb2_hsphy_vreg_names[] = { > + "vdd", "vdda12", > +}; > + > +#define EUSB2_NUM_VREGS ARRAY_SIZE(eusb2_hsphy_vreg_names) > + > +struct snps_eusb2_phy_drvdata { > + int (*phy_init)(struct phy *p); > + const char * const *clk_names; > + int num_clks; > +}; > + > +struct snps_eusb2_hsphy { > + struct phy *phy; > + void __iomem *base; > + > + struct clk *ref_clk; > + struct clk_bulk_data *clks; > + > + struct reset_control *phy_reset; > + > + struct regulator_bulk_data vregs[EUSB2_NUM_VREGS]; > + > + enum phy_mode mode; > + > + struct phy *repeater; > + > + const struct snps_eusb2_phy_drvdata *data; > +}; > + > +static int snps_eusb2_hsphy_set_mode(struct phy *p, enum phy_mode mode, int submode) > +{ > + struct snps_eusb2_hsphy *phy = phy_get_drvdata(p); > + > + phy->mode = mode; > + > + return phy_set_mode_ext(phy->repeater, mode, submode); > +} > + > +static void snps_eusb2_hsphy_write_mask(void __iomem *base, u32 offset, > + u32 mask, u32 val) > +{ > + u32 reg; > + > + reg = readl_relaxed(base + offset); > + reg &= ~mask; > + reg |= val & mask; > + writel_relaxed(reg, base + offset); > + > + /* Ensure above write is completed */ > + readl_relaxed(base + offset); > +} > + > +static void qcom_eusb2_default_parameters(struct snps_eusb2_hsphy *phy) > +{ > + /* default parameters: tx pre-emphasis */ > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_9, > + PHY_CFG_TX_PREEMP_TUNE_MASK, > + FIELD_PREP(PHY_CFG_TX_PREEMP_TUNE_MASK, 0)); > + > + /* tx rise/fall time */ > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_9, > + PHY_CFG_TX_RISE_TUNE_MASK, > + FIELD_PREP(PHY_CFG_TX_RISE_TUNE_MASK, 0x2)); > + > + /* source impedance adjustment */ > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_9, > + PHY_CFG_TX_RES_TUNE_MASK, > + FIELD_PREP(PHY_CFG_TX_RES_TUNE_MASK, 0x1)); > + > + /* dc voltage level adjustement */ > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_8, > + PHY_CFG_TX_HS_VREF_TUNE_MASK, > + FIELD_PREP(PHY_CFG_TX_HS_VREF_TUNE_MASK, 0x3)); > + > + /* transmitter HS crossover adjustement */ > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_8, > + PHY_CFG_TX_HS_XV_TUNE_MASK, > + FIELD_PREP(PHY_CFG_TX_HS_XV_TUNE_MASK, 0x0)); > +} > + > +static int qcom_eusb2_ref_clk_init(struct snps_eusb2_hsphy *phy) > +{ > + unsigned long ref_clk_freq = clk_get_rate(phy->ref_clk); > + > + switch (ref_clk_freq) { > + case 19200000: > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_HS_PHY_CTRL_COMMON0, > + FSEL_MASK, > + FIELD_PREP(FSEL_MASK, FSEL_19_2_MHZ_VAL)); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_2, > + PHY_CFG_PLL_FB_DIV_7_0_MASK, > + DIV_7_0_19_2_MHZ_VAL); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_3, > + PHY_CFG_PLL_FB_DIV_11_8_MASK, > + DIV_11_8_19_2_MHZ_VAL); > + break; > + > + case 38400000: > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_HS_PHY_CTRL_COMMON0, > + FSEL_MASK, > + FIELD_PREP(FSEL_MASK, FSEL_38_4_MHZ_VAL)); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_2, > + PHY_CFG_PLL_FB_DIV_7_0_MASK, > + DIV_7_0_38_4_MHZ_VAL); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_3, > + PHY_CFG_PLL_FB_DIV_11_8_MASK, > + DIV_11_8_38_4_MHZ_VAL); > + break; > + > + default: > + dev_err(&phy->phy->dev, "unsupported ref_clk_freq:%lu\n", ref_clk_freq); > + return -EINVAL; > + } > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_3, > + PHY_CFG_PLL_REF_DIV, PLL_REF_DIV_VAL); > + > + return 0; > +} > + > +static int qcom_snps_eusb2_hsphy_init(struct phy *p) > +{ > + struct snps_eusb2_hsphy *phy = phy_get_drvdata(p); > + int ret; > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG0, > + CMN_CTRL_OVERRIDE_EN, CMN_CTRL_OVERRIDE_EN); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_UTMI_CTRL5, POR, POR); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_HS_PHY_CTRL_COMMON0, > + PHY_ENABLE | RETENABLEN, PHY_ENABLE | RETENABLEN); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_APB_ACCESS_CMD, > + APB_LOGIC_RESET, APB_LOGIC_RESET); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_UTMI_PHY_CMN_CTRL0, TESTBURNIN, 0); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_FSEL_SEL, > + FSEL_SEL, FSEL_SEL); > + > + /* update ref_clk related registers */ > + ret = qcom_eusb2_ref_clk_init(phy); > + if (ret) > + return ret; > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_1, > + PHY_CFG_PLL_CPBIAS_CNTRL_MASK, > + FIELD_PREP(PHY_CFG_PLL_CPBIAS_CNTRL_MASK, 0x1)); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_4, > + PHY_CFG_PLL_INT_CNTRL_MASK, > + FIELD_PREP(PHY_CFG_PLL_INT_CNTRL_MASK, 0x8)); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_4, > + PHY_CFG_PLL_GMP_CNTRL_MASK, > + FIELD_PREP(PHY_CFG_PLL_GMP_CNTRL_MASK, 0x1)); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_5, > + PHY_CFG_PLL_PROP_CNTRL_MASK, > + FIELD_PREP(PHY_CFG_PLL_PROP_CNTRL_MASK, 0x10)); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_6, > + PHY_CFG_PLL_VCO_CNTRL_MASK, > + FIELD_PREP(PHY_CFG_PLL_VCO_CNTRL_MASK, 0x0)); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_5, > + PHY_CFG_PLL_VREF_TUNE_MASK, > + FIELD_PREP(PHY_CFG_PLL_VREF_TUNE_MASK, 0x1)); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_HS_PHY_CTRL2, > + VBUS_DET_EXT_SEL, VBUS_DET_EXT_SEL); > + > + /* set default parameters */ > + qcom_eusb2_default_parameters(phy); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_HS_PHY_CTRL2, > + USB2_SUSPEND_N_SEL | USB2_SUSPEND_N, > + USB2_SUSPEND_N_SEL | USB2_SUSPEND_N); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_UTMI_CTRL0, SLEEPM, SLEEPM); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_HS_PHY_CTRL_COMMON0, > + SIDDQ_SEL, SIDDQ_SEL); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_HS_PHY_CTRL_COMMON0, > + SIDDQ, 0); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_UTMI_CTRL5, POR, 0); > + > + snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_HS_PHY_CTRL2, > + USB2_SUSPEND_N_SEL, 0); > + > + return 0; > +} > + > +static const char * const qcom_eusb2_hsphy_clock_names[] = { > + "ref", > +}; > + > +static const struct snps_eusb2_phy_drvdata sm8550_snps_eusb2_phy = { > + .phy_init = qcom_snps_eusb2_hsphy_init, > + .clk_names = qcom_eusb2_hsphy_clock_names, > + .num_clks = ARRAY_SIZE(qcom_eusb2_hsphy_clock_names), > +}; > + > +static int snps_eusb2_hsphy_init(struct phy *p) > +{ > + struct snps_eusb2_hsphy *phy = phy_get_drvdata(p); > + int ret; > + > + ret = regulator_bulk_enable(ARRAY_SIZE(phy->vregs), phy->vregs); > + if (ret) > + return ret; > + > + ret = phy_init(phy->repeater); > + if (ret) { > + dev_err(&p->dev, "repeater init failed. %d\n", ret); > + goto disable_vreg; > + } > + > + ret = clk_prepare_enable(phy->ref_clk); > + if (ret) { > + dev_err(&p->dev, "failed to enable ref clock, %d\n", ret); > + goto disable_vreg; > + } > + > + ret = reset_control_assert(phy->phy_reset); > + if (ret) { > + dev_err(&p->dev, "failed to assert phy_reset, %d\n", ret); > + goto disable_ref_clk; > + } > + > + usleep_range(100, 150); > + > + ret = reset_control_deassert(phy->phy_reset); > + if (ret) { > + dev_err(&p->dev, "failed to de-assert phy_reset, %d\n", ret); > + goto disable_ref_clk; > + } > + > + ret = phy->data->phy_init(p); > + if (ret) > + goto disable_ref_clk; > + > + return 0; > + > +disable_ref_clk: > + clk_disable_unprepare(phy->ref_clk); > + > +disable_vreg: > + regulator_bulk_disable(ARRAY_SIZE(phy->vregs), phy->vregs); > + > + return ret; > +} > + > +static int snps_eusb2_hsphy_exit(struct phy *p) > +{ > + struct snps_eusb2_hsphy *phy = phy_get_drvdata(p); > + > + clk_disable_unprepare(phy->ref_clk); > + > + regulator_bulk_disable(ARRAY_SIZE(phy->vregs), phy->vregs); > + > + phy_exit(phy->repeater); > + > + return 0; > +} > + > +static const struct phy_ops snps_eusb2_hsphy_ops = { > + .init = snps_eusb2_hsphy_init, > + .exit = snps_eusb2_hsphy_exit, > + .set_mode = snps_eusb2_hsphy_set_mode, > + .owner = THIS_MODULE, > +}; > + > +static int snps_eusb2_hsphy_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct snps_eusb2_hsphy *phy; > + struct phy_provider *phy_provider; > + struct phy *generic_phy; > + const struct snps_eusb2_phy_drvdata *drv_data; > + int ret, i; > + int num; > + > + phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); > + if (!phy) > + return -ENOMEM; > + > + drv_data = of_device_get_match_data(dev); > + if (!drv_data) > + return -EINVAL; > + phy->data = drv_data; > + > + phy->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(phy->base)) > + return PTR_ERR(phy->base); > + > + phy->phy_reset = devm_reset_control_get_exclusive(dev, NULL); > + if (IS_ERR(phy->phy_reset)) > + return PTR_ERR(phy->phy_reset); > + > + phy->clks = devm_kcalloc(dev, > + phy->data->num_clks, > + sizeof(*phy->clks), > + GFP_KERNEL); > + if (!phy->clks) > + return -ENOMEM; > + > + for (int i = 0; i < phy->data->num_clks; ++i) > + phy->clks[i].id = phy->data->clk_names[i]; > + > + ret = devm_clk_bulk_get(dev, phy->data->num_clks, > + phy->clks); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to get phy clock(s)\n"); > + > + phy->ref_clk = NULL; > + for (int i = 0; i < phy->data->num_clks; ++i) { > + if (!strcmp(phy->clks[i].id, "ref")) { > + phy->ref_clk = phy->clks[i].clk; > + break; > + } > + } > + > + if (IS_ERR_OR_NULL(phy->ref_clk)) > + return dev_err_probe(dev, PTR_ERR(phy->ref_clk), > + "failed to get ref clk\n"); > + > + num = ARRAY_SIZE(phy->vregs); > + for (i = 0; i < num; i++) > + phy->vregs[i].supply = eusb2_hsphy_vreg_names[i]; > + > + ret = devm_regulator_bulk_get(dev, num, phy->vregs); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to get regulator supplies\n"); > + > + phy->repeater = devm_of_phy_get_by_index(dev, np, 0); > + if (IS_ERR(phy->repeater)) > + return dev_err_probe(dev, PTR_ERR(phy->repeater), > + "failed to get repeater\n"); > + > + generic_phy = devm_phy_create(dev, NULL, &snps_eusb2_hsphy_ops); > + if (IS_ERR(generic_phy)) { > + dev_err(dev, "failed to create phy %d\n", ret); > + return PTR_ERR(generic_phy); > + } > + > + dev_set_drvdata(dev, phy); > + phy_set_drvdata(generic_phy, phy); > + > + phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > + if (IS_ERR(phy_provider)) > + return PTR_ERR(phy_provider); > + > + dev_info(dev, "Registered Snps-eUSB2 phy\n"); > + > + return 0; > +} > + > +static const struct of_device_id snps_eusb2_hsphy_of_match_table[] = { > + { > + .compatible = "qcom,sm8550-snps-eusb2-phy", > + .data = &sm8550_snps_eusb2_phy, > + }, { }, > +}; > +MODULE_DEVICE_TABLE(of, snps_eusb2_hsphy_of_match_table); > + > +static struct platform_driver snps_eusb2_hsphy_driver = { > + .probe = snps_eusb2_hsphy_probe, > + .driver = { > + .name = "snps-eusb2-hsphy", > + .of_match_table = snps_eusb2_hsphy_of_match_table, > + }, > +}; > + > +module_platform_driver(snps_eusb2_hsphy_driver); > +MODULE_DESCRIPTION("SNPS eUSB2 HS PHY driver"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig > index 846f8c995..914547068 100644 > --- a/drivers/phy/qualcomm/Kconfig > +++ b/drivers/phy/qualcomm/Kconfig > @@ -125,15 +125,6 @@ config PHY_QCOM_QUSB2 > PHY which is usually paired with either the ChipIdea or Synopsys DWC3 > USB IPs on MSM SOCs. > > -config PHY_QCOM_SNPS_EUSB2 > - tristate "Qualcomm SNPS eUSB2 PHY Driver" > - depends on OF && (ARCH_QCOM || COMPILE_TEST) > - select GENERIC_PHY > - help > - Enable support for the USB high-speed SNPS eUSB2 phy on Qualcomm > - chipsets. The PHY is paired with a Synopsys DWC3 USB controller > - on Qualcomm SOCs. > - > config PHY_QCOM_EUSB2_REPEATER > tristate "Qualcomm SNPS eUSB2 Repeater Driver" > depends on OF && (ARCH_QCOM || COMPILE_TEST) > diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile > index eb60e950a..2121e92df 100644 > --- a/drivers/phy/qualcomm/Makefile > +++ b/drivers/phy/qualcomm/Makefile > @@ -15,7 +15,6 @@ obj-$(CONFIG_PHY_QCOM_QMP_USB) += phy-qcom-qmp-usb.o > obj-$(CONFIG_PHY_QCOM_QMP_USB_LEGACY) += phy-qcom-qmp-usb-legacy.o > > obj-$(CONFIG_PHY_QCOM_QUSB2) += phy-qcom-qusb2.o > -obj-$(CONFIG_PHY_QCOM_SNPS_EUSB2) += phy-qcom-snps-eusb2.o > obj-$(CONFIG_PHY_QCOM_EUSB2_REPEATER) += phy-qcom-eusb2-repeater.o > obj-$(CONFIG_PHY_QCOM_USB_HS) += phy-qcom-usb-hs.o > obj-$(CONFIG_PHY_QCOM_USB_HSIC) += phy-qcom-usb-hsic.o > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c b/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c > deleted file mode 100644 > index 1484691a4..000000000 > --- a/drivers/phy/qualcomm/phy-qcom-snps-eusb2.c > +++ /dev/null > @@ -1,442 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * Copyright (c) 2023, Linaro Limited > - */ > - > -#include <linux/bitfield.h> > -#include <linux/clk.h> > -#include <linux/delay.h> > -#include <linux/iopoll.h> > -#include <linux/mod_devicetable.h> > -#include <linux/phy/phy.h> > -#include <linux/platform_device.h> > -#include <linux/regulator/consumer.h> > -#include <linux/reset.h> > - > -#define USB_PHY_UTMI_CTRL0 (0x3c) > -#define SLEEPM BIT(0) > -#define OPMODE_MASK GENMASK(4, 3) > -#define OPMODE_NONDRIVING BIT(3) > - > -#define USB_PHY_UTMI_CTRL5 (0x50) > -#define POR BIT(1) > - > -#define USB_PHY_HS_PHY_CTRL_COMMON0 (0x54) > -#define PHY_ENABLE BIT(0) > -#define SIDDQ_SEL BIT(1) > -#define SIDDQ BIT(2) > -#define RETENABLEN BIT(3) > -#define FSEL_MASK GENMASK(6, 4) > -#define FSEL_19_2_MHZ_VAL (0x0) > -#define FSEL_38_4_MHZ_VAL (0x4) > - > -#define USB_PHY_CFG_CTRL_1 (0x58) > -#define PHY_CFG_PLL_CPBIAS_CNTRL_MASK GENMASK(7, 1) > - > -#define USB_PHY_CFG_CTRL_2 (0x5c) > -#define PHY_CFG_PLL_FB_DIV_7_0_MASK GENMASK(7, 0) > -#define DIV_7_0_19_2_MHZ_VAL (0x90) > -#define DIV_7_0_38_4_MHZ_VAL (0xc8) > - > -#define USB_PHY_CFG_CTRL_3 (0x60) > -#define PHY_CFG_PLL_FB_DIV_11_8_MASK GENMASK(3, 0) > -#define DIV_11_8_19_2_MHZ_VAL (0x1) > -#define DIV_11_8_38_4_MHZ_VAL (0x0) > - > -#define PHY_CFG_PLL_REF_DIV GENMASK(7, 4) > -#define PLL_REF_DIV_VAL (0x0) > - > -#define USB_PHY_HS_PHY_CTRL2 (0x64) > -#define VBUSVLDEXT0 BIT(0) > -#define USB2_SUSPEND_N BIT(2) > -#define USB2_SUSPEND_N_SEL BIT(3) > -#define VBUS_DET_EXT_SEL BIT(4) > - > -#define USB_PHY_CFG_CTRL_4 (0x68) > -#define PHY_CFG_PLL_GMP_CNTRL_MASK GENMASK(1, 0) > -#define PHY_CFG_PLL_INT_CNTRL_MASK GENMASK(7, 2) > - > -#define USB_PHY_CFG_CTRL_5 (0x6c) > -#define PHY_CFG_PLL_PROP_CNTRL_MASK GENMASK(4, 0) > -#define PHY_CFG_PLL_VREF_TUNE_MASK GENMASK(7, 6) > - > -#define USB_PHY_CFG_CTRL_6 (0x70) > -#define PHY_CFG_PLL_VCO_CNTRL_MASK GENMASK(2, 0) > - > -#define USB_PHY_CFG_CTRL_7 (0x74) > - > -#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) > - > -#define USB_PHY_CFG_CTRL_10 (0x80) > - > -#define USB_PHY_CFG0 (0x94) > -#define DATAPATH_CTRL_OVERRIDE_EN BIT(0) > -#define CMN_CTRL_OVERRIDE_EN BIT(1) > - > -#define UTMI_PHY_CMN_CTRL0 (0x98) > -#define TESTBURNIN BIT(6) > - > -#define USB_PHY_FSEL_SEL (0xb8) > -#define FSEL_SEL BIT(0) > - > -#define USB_PHY_APB_ACCESS_CMD (0x130) > -#define RW_ACCESS BIT(0) > -#define APB_START_CMD BIT(1) > -#define APB_LOGIC_RESET BIT(2) > - > -#define USB_PHY_APB_ACCESS_STATUS (0x134) > -#define ACCESS_DONE BIT(0) > -#define TIMED_OUT BIT(1) > -#define ACCESS_ERROR BIT(2) > -#define ACCESS_IN_PROGRESS BIT(3) > - > -#define USB_PHY_APB_ADDRESS (0x138) > -#define APB_REG_ADDR_MASK GENMASK(7, 0) > - > -#define USB_PHY_APB_WRDATA_LSB (0x13c) > -#define APB_REG_WRDATA_7_0_MASK GENMASK(3, 0) > - > -#define USB_PHY_APB_WRDATA_MSB (0x140) > -#define APB_REG_WRDATA_15_8_MASK GENMASK(7, 4) > - > -#define USB_PHY_APB_RDDATA_LSB (0x144) > -#define APB_REG_RDDATA_7_0_MASK GENMASK(3, 0) > - > -#define USB_PHY_APB_RDDATA_MSB (0x148) > -#define APB_REG_RDDATA_15_8_MASK GENMASK(7, 4) > - > -static const char * const eusb2_hsphy_vreg_names[] = { > - "vdd", "vdda12", > -}; > - > -#define EUSB2_NUM_VREGS ARRAY_SIZE(eusb2_hsphy_vreg_names) > - > -struct qcom_snps_eusb2_hsphy { > - struct phy *phy; > - void __iomem *base; > - > - struct clk *ref_clk; > - struct reset_control *phy_reset; > - > - struct regulator_bulk_data vregs[EUSB2_NUM_VREGS]; > - > - enum phy_mode mode; > - > - struct phy *repeater; > -}; > - > -static int qcom_snps_eusb2_hsphy_set_mode(struct phy *p, enum phy_mode mode, int submode) > -{ > - struct qcom_snps_eusb2_hsphy *phy = phy_get_drvdata(p); > - > - phy->mode = mode; > - > - return phy_set_mode_ext(phy->repeater, mode, submode); > -} > - > -static void qcom_snps_eusb2_hsphy_write_mask(void __iomem *base, u32 offset, > - u32 mask, u32 val) > -{ > - u32 reg; > - > - reg = readl_relaxed(base + offset); > - reg &= ~mask; > - reg |= val & mask; > - writel_relaxed(reg, base + offset); > - > - /* Ensure above write is completed */ > - readl_relaxed(base + offset); > -} > - > -static void qcom_eusb2_default_parameters(struct qcom_snps_eusb2_hsphy *phy) > -{ > - /* default parameters: tx pre-emphasis */ > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_9, > - PHY_CFG_TX_PREEMP_TUNE_MASK, > - FIELD_PREP(PHY_CFG_TX_PREEMP_TUNE_MASK, 0)); > - > - /* tx rise/fall time */ > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_9, > - PHY_CFG_TX_RISE_TUNE_MASK, > - FIELD_PREP(PHY_CFG_TX_RISE_TUNE_MASK, 0x2)); > - > - /* source impedance adjustment */ > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_9, > - PHY_CFG_TX_RES_TUNE_MASK, > - FIELD_PREP(PHY_CFG_TX_RES_TUNE_MASK, 0x1)); > - > - /* dc voltage level adjustement */ > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_8, > - PHY_CFG_TX_HS_VREF_TUNE_MASK, > - FIELD_PREP(PHY_CFG_TX_HS_VREF_TUNE_MASK, 0x3)); > - > - /* transmitter HS crossover adjustement */ > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_8, > - PHY_CFG_TX_HS_XV_TUNE_MASK, > - FIELD_PREP(PHY_CFG_TX_HS_XV_TUNE_MASK, 0x0)); > -} > - > -static int qcom_eusb2_ref_clk_init(struct qcom_snps_eusb2_hsphy *phy) > -{ > - unsigned long ref_clk_freq = clk_get_rate(phy->ref_clk); > - > - switch (ref_clk_freq) { > - case 19200000: > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_HS_PHY_CTRL_COMMON0, > - FSEL_MASK, > - FIELD_PREP(FSEL_MASK, FSEL_19_2_MHZ_VAL)); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_2, > - PHY_CFG_PLL_FB_DIV_7_0_MASK, > - DIV_7_0_19_2_MHZ_VAL); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_3, > - PHY_CFG_PLL_FB_DIV_11_8_MASK, > - DIV_11_8_19_2_MHZ_VAL); > - break; > - > - case 38400000: > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_HS_PHY_CTRL_COMMON0, > - FSEL_MASK, > - FIELD_PREP(FSEL_MASK, FSEL_38_4_MHZ_VAL)); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_2, > - PHY_CFG_PLL_FB_DIV_7_0_MASK, > - DIV_7_0_38_4_MHZ_VAL); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_3, > - PHY_CFG_PLL_FB_DIV_11_8_MASK, > - DIV_11_8_38_4_MHZ_VAL); > - break; > - > - default: > - dev_err(&phy->phy->dev, "unsupported ref_clk_freq:%lu\n", ref_clk_freq); > - return -EINVAL; > - } > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_3, > - PHY_CFG_PLL_REF_DIV, PLL_REF_DIV_VAL); > - > - return 0; > -} > - > -static int qcom_snps_eusb2_hsphy_init(struct phy *p) > -{ > - struct qcom_snps_eusb2_hsphy *phy = phy_get_drvdata(p); > - int ret; > - > - ret = regulator_bulk_enable(ARRAY_SIZE(phy->vregs), phy->vregs); > - if (ret) > - return ret; > - > - ret = phy_init(phy->repeater); > - if (ret) { > - dev_err(&p->dev, "repeater init failed. %d\n", ret); > - goto disable_vreg; > - } > - > - ret = clk_prepare_enable(phy->ref_clk); > - if (ret) { > - dev_err(&p->dev, "failed to enable ref clock, %d\n", ret); > - goto disable_vreg; > - } > - > - ret = reset_control_assert(phy->phy_reset); > - if (ret) { > - dev_err(&p->dev, "failed to assert phy_reset, %d\n", ret); > - goto disable_ref_clk; > - } > - > - usleep_range(100, 150); > - > - ret = reset_control_deassert(phy->phy_reset); > - if (ret) { > - dev_err(&p->dev, "failed to de-assert phy_reset, %d\n", ret); > - goto disable_ref_clk; > - } > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG0, > - CMN_CTRL_OVERRIDE_EN, CMN_CTRL_OVERRIDE_EN); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_UTMI_CTRL5, POR, POR); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_HS_PHY_CTRL_COMMON0, > - PHY_ENABLE | RETENABLEN, PHY_ENABLE | RETENABLEN); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_APB_ACCESS_CMD, > - APB_LOGIC_RESET, APB_LOGIC_RESET); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, UTMI_PHY_CMN_CTRL0, TESTBURNIN, 0); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_FSEL_SEL, > - FSEL_SEL, FSEL_SEL); > - > - /* update ref_clk related registers */ > - ret = qcom_eusb2_ref_clk_init(phy); > - if (ret) > - goto disable_ref_clk; > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_1, > - PHY_CFG_PLL_CPBIAS_CNTRL_MASK, > - FIELD_PREP(PHY_CFG_PLL_CPBIAS_CNTRL_MASK, 0x1)); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_4, > - PHY_CFG_PLL_INT_CNTRL_MASK, > - FIELD_PREP(PHY_CFG_PLL_INT_CNTRL_MASK, 0x8)); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_4, > - PHY_CFG_PLL_GMP_CNTRL_MASK, > - FIELD_PREP(PHY_CFG_PLL_GMP_CNTRL_MASK, 0x1)); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_5, > - PHY_CFG_PLL_PROP_CNTRL_MASK, > - FIELD_PREP(PHY_CFG_PLL_PROP_CNTRL_MASK, 0x10)); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_6, > - PHY_CFG_PLL_VCO_CNTRL_MASK, > - FIELD_PREP(PHY_CFG_PLL_VCO_CNTRL_MASK, 0x0)); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_CFG_CTRL_5, > - PHY_CFG_PLL_VREF_TUNE_MASK, > - FIELD_PREP(PHY_CFG_PLL_VREF_TUNE_MASK, 0x1)); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_HS_PHY_CTRL2, > - VBUS_DET_EXT_SEL, VBUS_DET_EXT_SEL); > - > - /* set default parameters */ > - qcom_eusb2_default_parameters(phy); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_HS_PHY_CTRL2, > - USB2_SUSPEND_N_SEL | USB2_SUSPEND_N, > - USB2_SUSPEND_N_SEL | USB2_SUSPEND_N); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_UTMI_CTRL0, SLEEPM, SLEEPM); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_HS_PHY_CTRL_COMMON0, > - SIDDQ_SEL, SIDDQ_SEL); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_HS_PHY_CTRL_COMMON0, > - SIDDQ, 0); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_UTMI_CTRL5, POR, 0); > - > - qcom_snps_eusb2_hsphy_write_mask(phy->base, USB_PHY_HS_PHY_CTRL2, > - USB2_SUSPEND_N_SEL, 0); > - > - return 0; > - > -disable_ref_clk: > - clk_disable_unprepare(phy->ref_clk); > - > -disable_vreg: > - regulator_bulk_disable(ARRAY_SIZE(phy->vregs), phy->vregs); > - > - return ret; > -} > - > -static int qcom_snps_eusb2_hsphy_exit(struct phy *p) > -{ > - struct qcom_snps_eusb2_hsphy *phy = phy_get_drvdata(p); > - > - clk_disable_unprepare(phy->ref_clk); > - > - regulator_bulk_disable(ARRAY_SIZE(phy->vregs), phy->vregs); > - > - phy_exit(phy->repeater); > - > - return 0; > -} > - > -static const struct phy_ops qcom_snps_eusb2_hsphy_ops = { > - .init = qcom_snps_eusb2_hsphy_init, > - .exit = qcom_snps_eusb2_hsphy_exit, > - .set_mode = qcom_snps_eusb2_hsphy_set_mode, > - .owner = THIS_MODULE, > -}; > - > -static int qcom_snps_eusb2_hsphy_probe(struct platform_device *pdev) > -{ > - struct device *dev = &pdev->dev; > - struct device_node *np = dev->of_node; > - struct qcom_snps_eusb2_hsphy *phy; > - struct phy_provider *phy_provider; > - struct phy *generic_phy; > - int ret, i; > - int num; > - > - phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL); > - if (!phy) > - return -ENOMEM; > - > - phy->base = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(phy->base)) > - return PTR_ERR(phy->base); > - > - phy->phy_reset = devm_reset_control_get_exclusive(dev, NULL); > - if (IS_ERR(phy->phy_reset)) > - return PTR_ERR(phy->phy_reset); > - > - phy->ref_clk = devm_clk_get(dev, "ref"); > - if (IS_ERR(phy->ref_clk)) > - return dev_err_probe(dev, PTR_ERR(phy->ref_clk), > - "failed to get ref clk\n"); > - > - num = ARRAY_SIZE(phy->vregs); > - for (i = 0; i < num; i++) > - phy->vregs[i].supply = eusb2_hsphy_vreg_names[i]; > - > - ret = devm_regulator_bulk_get(dev, num, phy->vregs); > - if (ret) > - return dev_err_probe(dev, ret, > - "failed to get regulator supplies\n"); > - > - phy->repeater = devm_of_phy_get_by_index(dev, np, 0); > - if (IS_ERR(phy->repeater)) > - return dev_err_probe(dev, PTR_ERR(phy->repeater), > - "failed to get repeater\n"); > - > - generic_phy = devm_phy_create(dev, NULL, &qcom_snps_eusb2_hsphy_ops); > - if (IS_ERR(generic_phy)) { > - dev_err(dev, "failed to create phy %d\n", ret); > - return PTR_ERR(generic_phy); > - } > - > - dev_set_drvdata(dev, phy); > - phy_set_drvdata(generic_phy, phy); > - > - phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > - if (IS_ERR(phy_provider)) > - return PTR_ERR(phy_provider); > - > - dev_info(dev, "Registered Qcom-eUSB2 phy\n"); > - > - return 0; > -} > - > -static const struct of_device_id qcom_snps_eusb2_hsphy_of_match_table[] = { > - { .compatible = "qcom,sm8550-snps-eusb2-phy", }, > - { }, > -}; > -MODULE_DEVICE_TABLE(of, qcom_snps_eusb2_hsphy_of_match_table); > - > -static struct platform_driver qcom_snps_eusb2_hsphy_driver = { > - .probe = qcom_snps_eusb2_hsphy_probe, > - .driver = { > - .name = "qcom-snps-eusb2-hsphy", > - .of_match_table = qcom_snps_eusb2_hsphy_of_match_table, > - }, > -}; > - > -module_platform_driver(qcom_snps_eusb2_hsphy_driver); > -MODULE_DESCRIPTION("Qualcomm SNPS eUSB2 HS PHY driver"); > -MODULE_LICENSE("GPL");
On 24/02/2025 11:48, Ivaylo Ivanov wrote: > On 2/24/25 10:56, Krzysztof Kozlowski wrote: >> On Sun, Feb 23, 2025 at 02:22:22PM +0200, Ivaylo Ivanov wrote: >>> The Exynos2200 SoC has a USB controller PHY, which acts as an >>> intermediary between a USB controller (typically DWC3) and other PHYs >>> (UTMI, PIPE3). Add a dt-binding schema for it. >>> >>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >>> --- >>> .../phy/samsung,exynos2200-usbcon-phy.yaml | 76 +++++++++++++++++++ >>> 1 file changed, 76 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >> You have undocumented dependencies which prevent merging this file. >> First, dependencies have to be clearly expressed. > > They are, in the cover letter. Where? I read it twice. Dependencies is the most important thing and should scream at beginning of the cover letter, so if you bury them somewhere deep it also would not matter - just like they were missing. > >> Second, you should >> rather decouple the code from header dependencies, otherwise this cannot >> be merged for current release (just use clocks with long names, without IDs). > > Sure > >> >>> diff --git a/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>> new file mode 100644 >>> index 000000000..7d879ec8b >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>> @@ -0,0 +1,76 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/phy/samsung,exynos2200-usbcon-phy.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Exynos2200 USB controller PHY >>> + >>> +maintainers: >>> + - Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >>> + >>> +description: >>> + Exynos2200 USB controller PHY is an intermediary between a USB controller >>> + (typically DWC3) and other PHYs (UTMI, PIPE3). >> Isn't this the same as usbdrd phy? see: samsung,usb3-drd-phy.yaml > > It's not (I think). There's a few reasons I've decided to make this separate > from the usb3-drd-phy bindings and exynos5-usbdrd driver: > > 1. This PHY does not provide UTMI and PIPE3 on its own. There's no tuning USBDRD phy does not provide UTMI and PIPE on its own either if you look at diagram - they call it phy controller. > for them, and all that is needed from it is to disable HWACG, assert/ > deassert reset and force bvalid/vbusvalid. After that SNPS eUSB2 > initialization can be done and USB2 works. If the USBCON phy is not set > up before the eUSB2 one, the device hangs, so there is definitely a > dependancy between them. For PIPE3 we'd need to control the pipe3 > attaching/deattaching and then initialize the synopsys USBDP combophy. Does it mean there is no USB DRD phy controller as before? Anyway the problem is you have DWC3 -> PHY -> PHY. Looks one phy too many. > > 2. With the way it's modelled, we need to parse phandles from eUSB2 and > USBDP to the controller. Adding that to the usbdrd driver would be... > weird. It makes more sense to model it as a separate driver, because > it functions in a different way. Just to be clear: we don't talk about drivers here. Best regards, Krzysztof
On Sun, Feb 23, 2025 at 02:22:20PM +0200, Ivaylo Ivanov wrote: > As Samsung has been using the same Synopsys eUSB2 IP in Exynos2200, > albeit with a different register layout, rename qcom,snps-eusb2-phy > to snps,eusb2-phy and drop mentions of it being only for Qualcomm SoCs > in the binding description. Rename itself is pointless. One logical change is: you add here samsung, this you rename it. Best regards, Krzysztof
On 3/3/25 09:24, Krzysztof Kozlowski wrote: > On 02/03/2025 10:16, Ivaylo Ivanov wrote: >> On 2/25/25 10:11, Krzysztof Kozlowski wrote: >>> On 24/02/2025 11:48, Ivaylo Ivanov wrote: >>>> On 2/24/25 10:56, Krzysztof Kozlowski wrote: >>>>> On Sun, Feb 23, 2025 at 02:22:22PM +0200, Ivaylo Ivanov wrote: >>>>>> The Exynos2200 SoC has a USB controller PHY, which acts as an >>>>>> intermediary between a USB controller (typically DWC3) and other PHYs >>>>>> (UTMI, PIPE3). Add a dt-binding schema for it. >>>>>> >>>>>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >>>>>> --- >>>>>> .../phy/samsung,exynos2200-usbcon-phy.yaml | 76 +++++++++++++++++++ >>>>>> 1 file changed, 76 insertions(+) >>>>>> create mode 100644 Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>> You have undocumented dependencies which prevent merging this file. >>>>> First, dependencies have to be clearly expressed. >>>> They are, in the cover letter. >>> Where? I read it twice. Dependencies is the most important thing and >>> should scream at beginning of the cover letter, so if you bury them >>> somewhere deep it also would not matter - just like they were missing. >>> >>>>> Second, you should >>>>> rather decouple the code from header dependencies, otherwise this cannot >>>>> be merged for current release (just use clocks with long names, without IDs). >>>> Sure >>>>>> diff --git a/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>>> new file mode 100644 >>>>>> index 000000000..7d879ec8b >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>>> @@ -0,0 +1,76 @@ >>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>> +%YAML 1.2 >>>>>> +--- >>>>>> +$id: http://devicetree.org/schemas/phy/samsung,exynos2200-usbcon-phy.yaml# >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>> + >>>>>> +title: Exynos2200 USB controller PHY >>>>>> + >>>>>> +maintainers: >>>>>> + - Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >>>>>> + >>>>>> +description: >>>>>> + Exynos2200 USB controller PHY is an intermediary between a USB controller >>>>>> + (typically DWC3) and other PHYs (UTMI, PIPE3). >>>>> Isn't this the same as usbdrd phy? see: samsung,usb3-drd-phy.yaml >>>> It's not (I think). There's a few reasons I've decided to make this separate >>>> from the usb3-drd-phy bindings and exynos5-usbdrd driver: >>>> >>>> 1. This PHY does not provide UTMI and PIPE3 on its own. There's no tuning >>> USBDRD phy does not provide UTMI and PIPE on its own either if you look >>> at diagram - they call it phy controller. >> Ughm. What? So in most exynos cases, there's a combination of multiple phys? > >>>> for them, and all that is needed from it is to disable HWACG, assert/ >>>> deassert reset and force bvalid/vbusvalid. After that SNPS eUSB2 >>>> initialization can be done and USB2 works. If the USBCON phy is not set >>>> up before the eUSB2 one, the device hangs, so there is definitely a >>>> dependancy between them. For PIPE3 we'd need to control the pipe3 >>>> attaching/deattaching and then initialize the synopsys USBDP combophy. >>> Does it mean there is no USB DRD phy controller as before? >>> >>> Anyway the problem is you have DWC3 -> PHY -> PHY. Looks one phy too many. >> So... >> >> DWC3 -> USBDRD (USBCON) -> PHYs? > No, drop last phy. You just wrote the same as me - two phys, because > usbdrd is the phy. In all existing designs there is no such controllable > object from the point of view of operating system. What? Per my understanding, the phy property should refer to whatever is is connected to dwc3 UTMI. In this case it's the so-called USBDRD phy (called usbcon in downstream). Considering that the eUSB2 IP definitely also has UTMI that has to be connected to something, doesn't that mean we have clearly separated hardware blocks? Now, I guess one could argue that this USBCON hardware block could be classified as a syscon. But I don't see the problem with the current binding description, nor the modelling, as it represents how the hardware is (unless I've gotten it completely wrong). Best regards, Ivaylo > >> ...with usbdrd controller connecting and controlling the USB2 and USB3 >> phys, as well as dual role mode? > Yes. > >> Well, where is the DRD part in the exynos5 >> driver? > DRD? I believe it is part of DWC3, the same as in every other standard > implementation of Synopsys DWC3. > >> I guess it does perfectly fit the job of a usbdrd controller then (if it >> even deals with DRD). But then again, this brings up two questions: >> 1. Should this driver even be named exynos2200-usbcon and not, for >> example, exynos2200-usbdrd? > Are you sure we talk about the same thing? USBDRD is IP block in the > Exynos and a device driver. Call your device as appropriate it is - > based on datasheet or downstream sources. > >> 2. Are the exynos5-usbdrd phys really only USBDRD, or do they implement >> USB speed functionality? What is the UTMI/PIPE3 setup for then? > Dunno, I don't get what you mean by "exynos5-usbdrd phys really only > USBDRD". USBDRD is just the name of the device. > > Best regards, > Krzysztof
On 3/4/25 09:21, Krzysztof Kozlowski wrote: > On 03/03/2025 18:18, Ivaylo Ivanov wrote: >> On 3/3/25 09:24, Krzysztof Kozlowski wrote: >>> On 02/03/2025 10:16, Ivaylo Ivanov wrote: >>>> On 2/25/25 10:11, Krzysztof Kozlowski wrote: >>>>> On 24/02/2025 11:48, Ivaylo Ivanov wrote: >>>>>> On 2/24/25 10:56, Krzysztof Kozlowski wrote: >>>>>>> On Sun, Feb 23, 2025 at 02:22:22PM +0200, Ivaylo Ivanov wrote: >>>>>>>> The Exynos2200 SoC has a USB controller PHY, which acts as an >>>>>>>> intermediary between a USB controller (typically DWC3) and other PHYs >>>>>>>> (UTMI, PIPE3). Add a dt-binding schema for it. >>>>>>>> >>>>>>>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >>>>>>>> --- >>>>>>>> .../phy/samsung,exynos2200-usbcon-phy.yaml | 76 +++++++++++++++++++ >>>>>>>> 1 file changed, 76 insertions(+) >>>>>>>> create mode 100644 Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>>>> You have undocumented dependencies which prevent merging this file. >>>>>>> First, dependencies have to be clearly expressed. >>>>>> They are, in the cover letter. >>>>> Where? I read it twice. Dependencies is the most important thing and >>>>> should scream at beginning of the cover letter, so if you bury them >>>>> somewhere deep it also would not matter - just like they were missing. >>>>> >>>>>>> Second, you should >>>>>>> rather decouple the code from header dependencies, otherwise this cannot >>>>>>> be merged for current release (just use clocks with long names, without IDs). >>>>>> Sure >>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>>>>> new file mode 100644 >>>>>>>> index 000000000..7d879ec8b >>>>>>>> --- /dev/null >>>>>>>> +++ b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>>>>> @@ -0,0 +1,76 @@ >>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>>>> +%YAML 1.2 >>>>>>>> +--- >>>>>>>> +$id: http://devicetree.org/schemas/phy/samsung,exynos2200-usbcon-phy.yaml# >>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>> + >>>>>>>> +title: Exynos2200 USB controller PHY >>>>>>>> + >>>>>>>> +maintainers: >>>>>>>> + - Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >>>>>>>> + >>>>>>>> +description: >>>>>>>> + Exynos2200 USB controller PHY is an intermediary between a USB controller >>>>>>>> + (typically DWC3) and other PHYs (UTMI, PIPE3). >>>>>>> Isn't this the same as usbdrd phy? see: samsung,usb3-drd-phy.yaml >>>>>> It's not (I think). There's a few reasons I've decided to make this separate >>>>>> from the usb3-drd-phy bindings and exynos5-usbdrd driver: >>>>>> >>>>>> 1. This PHY does not provide UTMI and PIPE3 on its own. There's no tuning >>>>> USBDRD phy does not provide UTMI and PIPE on its own either if you look >>>>> at diagram - they call it phy controller. >>>> Ughm. What? So in most exynos cases, there's a combination of multiple phys? >>>>>> for them, and all that is needed from it is to disable HWACG, assert/ >>>>>> deassert reset and force bvalid/vbusvalid. After that SNPS eUSB2 >>>>>> initialization can be done and USB2 works. If the USBCON phy is not set >>>>>> up before the eUSB2 one, the device hangs, so there is definitely a >>>>>> dependancy between them. For PIPE3 we'd need to control the pipe3 >>>>>> attaching/deattaching and then initialize the synopsys USBDP combophy. >>>>> Does it mean there is no USB DRD phy controller as before? >>>>> >>>>> Anyway the problem is you have DWC3 -> PHY -> PHY. Looks one phy too many. >>>> So... >>>> >>>> DWC3 -> USBDRD (USBCON) -> PHYs? >>> No, drop last phy. You just wrote the same as me - two phys, because >>> usbdrd is the phy. In all existing designs there is no such controllable >>> object from the point of view of operating system. >> What? Per my understanding, the phy property should refer to whatever is >> is connected to dwc3 UTMI. In this case it's the so-called USBDRD phy (called >> usbcon in downstream). Considering that the eUSB2 IP definitely also has UTMI >> that has to be connected to something, doesn't that mean we have clearly > The entire point is that eUSB2 is connected to DWC3, no? That's exactly > how it is done for example on Qualcomm SoC. Otherwise you claim that > DWC3 controls one phy, which controls another phy which controls UTMI... But where does the USBCON fit? Is it just a side controller? Why's it needed in the first place? This is what I don't understand. > >> separated hardware blocks? Now, I guess one could argue that this USBCON >> hardware block could be classified as a syscon. But I don't see the problem >> with the current binding description, nor the modelling, as it represents >> how the hardware is (unless I've gotten it completely wrong). > It is the first time you use argument that it represents how the > hardware is and this is what we actually disagree. It is not like that. > You do not have chain of phys. Just look at any USB 3.0 DRD DWC diagram > from any Samsung SoC: where would you squeeze these two phys in relation > to what is called there "USB 3.0 PHY" which would be the third phy (!!!). Yeah, my point was that it was different from any previous design. Now, I don't know if it's actually theoretically possible to design it like so. It's hard to just guess how the hardware is designed without having access to die shots, documentations or even just schematics. Let's make it clear now, the changes your request are to document USBCON in the existing exynos binding, as well as to correct all explanations of how this block functions, right? Best regards, Ivaylo > > Best regards, > Krzysztof
On 3/4/25 12:03, Krzysztof Kozlowski wrote: > On 04/03/2025 10:09, Ivaylo Ivanov wrote: >> On 3/4/25 09:21, Krzysztof Kozlowski wrote: >>> On 03/03/2025 18:18, Ivaylo Ivanov wrote: >>>> On 3/3/25 09:24, Krzysztof Kozlowski wrote: >>>>> On 02/03/2025 10:16, Ivaylo Ivanov wrote: >>>>>> On 2/25/25 10:11, Krzysztof Kozlowski wrote: >>>>>>> On 24/02/2025 11:48, Ivaylo Ivanov wrote: >>>>>>>> On 2/24/25 10:56, Krzysztof Kozlowski wrote: >>>>>>>>> On Sun, Feb 23, 2025 at 02:22:22PM +0200, Ivaylo Ivanov wrote: >>>>>>>>>> The Exynos2200 SoC has a USB controller PHY, which acts as an >>>>>>>>>> intermediary between a USB controller (typically DWC3) and other PHYs >>>>>>>>>> (UTMI, PIPE3). Add a dt-binding schema for it. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >>>>>>>>>> --- >>>>>>>>>> .../phy/samsung,exynos2200-usbcon-phy.yaml | 76 +++++++++++++++++++ >>>>>>>>>> 1 file changed, 76 insertions(+) >>>>>>>>>> create mode 100644 Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>>>>>> You have undocumented dependencies which prevent merging this file. >>>>>>>>> First, dependencies have to be clearly expressed. >>>>>>>> They are, in the cover letter. >>>>>>> Where? I read it twice. Dependencies is the most important thing and >>>>>>> should scream at beginning of the cover letter, so if you bury them >>>>>>> somewhere deep it also would not matter - just like they were missing. >>>>>>> >>>>>>>>> Second, you should >>>>>>>>> rather decouple the code from header dependencies, otherwise this cannot >>>>>>>>> be merged for current release (just use clocks with long names, without IDs). >>>>>>>> Sure >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>>>>>>> new file mode 100644 >>>>>>>>>> index 000000000..7d879ec8b >>>>>>>>>> --- /dev/null >>>>>>>>>> +++ b/Documentation/devicetree/bindings/phy/samsung,exynos2200-usbcon-phy.yaml >>>>>>>>>> @@ -0,0 +1,76 @@ >>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>>>>>> +%YAML 1.2 >>>>>>>>>> +--- >>>>>>>>>> +$id: http://devicetree.org/schemas/phy/samsung,exynos2200-usbcon-phy.yaml# >>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>>>> + >>>>>>>>>> +title: Exynos2200 USB controller PHY >>>>>>>>>> + >>>>>>>>>> +maintainers: >>>>>>>>>> + - Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> >>>>>>>>>> + >>>>>>>>>> +description: >>>>>>>>>> + Exynos2200 USB controller PHY is an intermediary between a USB controller >>>>>>>>>> + (typically DWC3) and other PHYs (UTMI, PIPE3). >>>>>>>>> Isn't this the same as usbdrd phy? see: samsung,usb3-drd-phy.yaml >>>>>>>> It's not (I think). There's a few reasons I've decided to make this separate >>>>>>>> from the usb3-drd-phy bindings and exynos5-usbdrd driver: >>>>>>>> >>>>>>>> 1. This PHY does not provide UTMI and PIPE3 on its own. There's no tuning >>>>>>> USBDRD phy does not provide UTMI and PIPE on its own either if you look >>>>>>> at diagram - they call it phy controller. >>>>>> Ughm. What? So in most exynos cases, there's a combination of multiple phys? >>>>>>>> for them, and all that is needed from it is to disable HWACG, assert/ >>>>>>>> deassert reset and force bvalid/vbusvalid. After that SNPS eUSB2 >>>>>>>> initialization can be done and USB2 works. If the USBCON phy is not set >>>>>>>> up before the eUSB2 one, the device hangs, so there is definitely a >>>>>>>> dependancy between them. For PIPE3 we'd need to control the pipe3 >>>>>>>> attaching/deattaching and then initialize the synopsys USBDP combophy. >>>>>>> Does it mean there is no USB DRD phy controller as before? >>>>>>> >>>>>>> Anyway the problem is you have DWC3 -> PHY -> PHY. Looks one phy too many. >>>>>> So... >>>>>> >>>>>> DWC3 -> USBDRD (USBCON) -> PHYs? >>>>> No, drop last phy. You just wrote the same as me - two phys, because >>>>> usbdrd is the phy. In all existing designs there is no such controllable >>>>> object from the point of view of operating system. >>>> What? Per my understanding, the phy property should refer to whatever is >>>> is connected to dwc3 UTMI. In this case it's the so-called USBDRD phy (called >>>> usbcon in downstream). Considering that the eUSB2 IP definitely also has UTMI >>>> that has to be connected to something, doesn't that mean we have clearly >>> The entire point is that eUSB2 is connected to DWC3, no? That's exactly >>> how it is done for example on Qualcomm SoC. Otherwise you claim that >>> DWC3 controls one phy, which controls another phy which controls UTMI... >> But where does the USBCON fit? Is it just a side controller? Why's it needed >> in the first place? This is what I don't understand. > I assume usbcon, so old usbdrd, is the second DWC3's phy, just like qcom > qmpphy. Ugh. For qcoms, does the first phy depend on qmpphy? If we pass it as the second phy, I don't know how the linkreset will happen. We also have a usbdp phy, which I suspect is used for SS as well since it's a combophy by Synopsys. https://gitlab.com/Mis012/sm-s908b-linux-source-code/-/blob/s22_restored_history/drivers/phy/samsung/phy-exynos-usbdrd-eusb.c#L1579 Best regards, Ivaylo > >>>> separated hardware blocks? Now, I guess one could argue that this USBCON >>>> hardware block could be classified as a syscon. But I don't see the problem >>>> with the current binding description, nor the modelling, as it represents >>>> how the hardware is (unless I've gotten it completely wrong). >>> It is the first time you use argument that it represents how the >>> hardware is and this is what we actually disagree. It is not like that. >>> You do not have chain of phys. Just look at any USB 3.0 DRD DWC diagram >>> from any Samsung SoC: where would you squeeze these two phys in relation >>> to what is called there "USB 3.0 PHY" which would be the third phy (!!!). >> Yeah, my point was that it was different from any previous design. Now, >> I don't know if it's actually theoretically possible to design it like so. It's >> hard to just guess how the hardware is designed without having access >> to die shots, documentations or even just schematics. >> >> Let's make it clear now, the changes your request are to document USBCON >> in the existing exynos binding, as well as to correct all explanations of how >> this block functions, right? > No, not necessarily. If USBCON is entirely different device than USBDRD > (different register layout, different features), then go ahead with a > new binding. > > I was questioning your chain of PHYs and this should be investigated. > > > Best regards, > Krzysztof