Message-ID: <CA+zupgwbmKt8BhHEM-76CLLH_dE_vmyHvKwqpC2WzwED9irEVw@mail.gmail.com>
Date: Mon, 20 Oct 2025 12:55:18 -0700
From: Roy Luo <royluo@...gle.com>
To: Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
Cc: Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Peter Griffin <peter.griffin@...aro.org>, 
	André Draszik <andre.draszik@...aro.org>, 
	Tudor Ambarus <tudor.ambarus@...aro.org>, Joy Chakraborty <joychakr@...gle.com>, 
	Naveen Kumar <mnkumar@...gle.com>, Badhri Jagan Sridharan <badhri@...gle.com>, linux-phy@...ts.infradead.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v4 2/2] phy: Add Google Tensor SoC USB PHY driver

On Sat, Oct 18, 2025 at 1:13 AM Ivaylo Ivanov
<ivo.ivanov.ivanov1@...il.com> wrote:
>
> On 10/18/25 02:51, Roy Luo wrote:
> > Support the USB PHY found on Google Tensor G5. This particular USB PHY
> > supports both high-speed and super-speed operations, and is integrated
> > with the SNPS DWC3 controller that's also on the SoC.
> > This initial patch specifically adds functionality for high-speed.
> >
> > Co-developed-by: Joy Chakraborty <joychakr@...gle.com>
> > Signed-off-by: Joy Chakraborty <joychakr@...gle.com>
> > Co-developed-by: Naveen Kumar <mnkumar@...gle.com>
> > Signed-off-by: Naveen Kumar <mnkumar@...gle.com>
> > Signed-off-by: Roy Luo <royluo@...gle.com>
> > ---
> >  drivers/phy/Kconfig          |  13 ++
> >  drivers/phy/Makefile         |   1 +
> >  drivers/phy/phy-google-usb.c | 271 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 285 insertions(+)
> >  create mode 100644 drivers/phy/phy-google-usb.c
> >
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index 58c911e1b2d2..fe32d1356002 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -101,6 +101,19 @@ config PHY_NXP_PTN3222
> >         schemes. It supports all three USB 2.0 data rates: Low Speed, Full
> >         Speed and High Speed.
> >
> > +config PHY_GOOGLE_USB
> > +     tristate "Google Tensor SoC USB PHY driver"
> > +     depends on HAS_IOMEM
> > +     depends on OF
> > +     depends on TYPEC
> > +     select GENERIC_PHY
> > +     help
> > +       Enable support for the USB PHY on Google Tensor SoCs, starting with
> > +       the G5 generation. This driver provides the PHY interfaces to
> > +       interact with the SNPS eUSB2 and USB 3.2/DisplayPort Combo PHY, both
> > +       of which are integrated with the DWC3 USB controller.
>
> So it's more like a DRD controller, since it encapsulates multiple phys?

Yes, it's a DRD controller, I will make it clear in the next version.

>
> > +       This driver currently supports USB high-speed.
> > +
> >  source "drivers/phy/allwinner/Kconfig"
> >  source "drivers/phy/amlogic/Kconfig"
> >  source "drivers/phy/broadcom/Kconfig"
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index c670a8dac468..1d7a1331bd19 100644
> ...
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/mutex.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/usb/typec_mux.h>
> > +
> > +#define USBCS_USB2PHY_CFG19_OFFSET 0x0
> > +#define USBCS_USB2PHY_CFG19_PHY_CFG_PLL_FB_DIV GENMASK(19, 8)
>
> I'd suggest implementing the eUSB support in the existing snps-eusb2
> driver for the sake of clarity. That way, you can pass it as a phandle
> to this driver and probe it when drd is probing.

If I understand it correctly, you're referring to the pattern used in
Documentation/devicetree/bindings/phy/samsung,exynos2200-eusb2-phy.yaml and
Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yaml.
Please correct me if I'm wrong.

At first glance, this approach seems feasible. Custom logic, such
as vbus valid handling, would need to stay within this driver. Most
of the eUSB block configuration in google_usb2_phy_init() could
be moved to the snps-eusb2 driver by adding one more compatible
for Google's version of the SNSP eUSB IP. However, I'd argue
1. This approach would introduce an extra layer of PHY interface,
    which adds unnecessary complexity.
2. Not much of the code is reused in snps-eusb2 driver: the register
    definition and programming sequence are specific to Google,
    hence the logic in google_usb2_phy_init() would have to be
    moved to snps-eusb2 driver almost word-by-word.

I understand this approach was suggested for the sake of clarity,
but I'm not sure whether that makes for the best trade-off. Since
there is a precedent for this approach, I do not have a strong
objection. It would be helpful if the maintainers could also provide
their input on this specific point.

>
> > +
> > +#define USBCS_USB2PHY_CFG21_OFFSET 0x8
> > +#define USBCS_USB2PHY_CFG21_PHY_ENABLE BIT(12)
> > +#define USBCS_USB2PHY_CFG21_REF_FREQ_SEL GENMASK(15, 13)
> > +#define USBCS_USB2PHY_CFG21_PHY_TX_DIG_BYPASS_SEL BIT(19)
> > +
> > +#define USBCS_PHY_CFG1_OFFSET 0x28
> > +#define USBCS_PHY_CFG1_SYS_VBUSVALID BIT(17)
> > +
> > +enum google_usb_phy_id {
> > +     GOOGLE_USB2_PHY,
> > +     GOOGLE_USB_PHY_NUM,
> > +};
> > +
> > +struct google_usb_phy_instance {
> > +     int index;
> > +     struct phy *phy;
> > +     int num_clks;
> > +     struct clk_bulk_data *clks;
> > +     struct reset_control *rsts;
> > +};
> > +
> > +struct google_usb_phy {
> > +     struct device *dev;
> > +     void __iomem *u2phy_cfg_base;
> > +     void __iomem *dp_top_base;
> > +     struct google_usb_phy_instance insts[GOOGLE_USB_PHY_NUM];
> > +     /* serialize phy access */
> > +     struct mutex phy_mutex;
> > +     struct typec_switch_dev *sw;
> > +     enum typec_orientation orientation;
> > +};
> > +
> > +static inline struct google_usb_phy *to_google_usb_phy(struct google_usb_phy_instance *inst)
> > +{
> > +     return container_of(inst, struct google_usb_phy, insts[inst->index]);
> > +}
> > +
> > +static void set_vbus_valid(struct google_usb_phy *gphy)
> > +{
> > +     u32 reg;
> > +
> > +     if (gphy->orientation == TYPEC_ORIENTATION_NONE) {
> > +             reg = readl(gphy->dp_top_base + USBCS_PHY_CFG1_OFFSET);
> > +             reg &= ~USBCS_PHY_CFG1_SYS_VBUSVALID;
> > +             writel(reg, gphy->dp_top_base + USBCS_PHY_CFG1_OFFSET);
> > +     } else {
> > +             reg = readl(gphy->dp_top_base + USBCS_PHY_CFG1_OFFSET);
> > +             reg |= USBCS_PHY_CFG1_SYS_VBUSVALID;
> > +             writel(reg, gphy->dp_top_base + USBCS_PHY_CFG1_OFFSET);
> > +     }
> > +}
> > +
> > +static int google_usb_set_orientation(struct typec_switch_dev *sw,
> > +                                   enum typec_orientation orientation)
> > +{
> > +     struct google_usb_phy *gphy = typec_switch_get_drvdata(sw);
> > +
> > +     dev_dbg(gphy->dev, "set orientation %d\n", orientation);
> > +
> > +     gphy->orientation = orientation;
> > +
> > +     if (pm_runtime_suspended(gphy->dev))
> > +             return 0;
> > +
> > +     guard(mutex)(&gphy->phy_mutex);
> > +
> > +     set_vbus_valid(gphy);
> > +
> > +     return 0;
> > +}
> > +
> > +static int google_usb2_phy_init(struct phy *_phy)
> > +{
> > +     struct google_usb_phy_instance *inst = phy_get_drvdata(_phy);
> > +     struct google_usb_phy *gphy = to_google_usb_phy(inst);
> > +     u32 reg;
> > +     int ret = 0;
> > +
> > +     dev_dbg(gphy->dev, "initializing usb2 phy\n");
> > +
> > +     guard(mutex)(&gphy->phy_mutex);
> > +
> > +     reg = readl(gphy->u2phy_cfg_base + USBCS_USB2PHY_CFG21_OFFSET);
> > +     reg &= ~USBCS_USB2PHY_CFG21_PHY_TX_DIG_BYPASS_SEL;
> > +     reg &= ~USBCS_USB2PHY_CFG21_REF_FREQ_SEL;
> > +     reg |= FIELD_PREP(USBCS_USB2PHY_CFG21_REF_FREQ_SEL, 0);
> > +     writel(reg, gphy->u2phy_cfg_base + USBCS_USB2PHY_CFG21_OFFSET);
> > +
> > +     reg = readl(gphy->u2phy_cfg_base + USBCS_USB2PHY_CFG19_OFFSET);
> > +     reg &= ~USBCS_USB2PHY_CFG19_PHY_CFG_PLL_FB_DIV;
> > +     reg |= FIELD_PREP(USBCS_USB2PHY_CFG19_PHY_CFG_PLL_FB_DIV, 368);
>
> Yeah, it's definitely the eUSB IP, but wired differently.
>
> phy-snps-eusb2.c:
> #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)
>
> > +     writel(reg, gphy->u2phy_cfg_base + USBCS_USB2PHY_CFG19_OFFSET);
> > +
> > +     set_vbus_valid(gphy);
> > +
> > +     ret = clk_bulk_prepare_enable(inst->num_clks, inst->clks);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = reset_control_deassert(inst->rsts);
> > +     if (ret) {
> > +             clk_bulk_disable_unprepare(inst->num_clks, inst->clks);
> > +             return ret;
> > +     }
> > +
> > +     reg = readl(gphy->u2phy_cfg_base + USBCS_USB2PHY_CFG21_OFFSET);
> > +     reg |= USBCS_USB2PHY_CFG21_PHY_ENABLE;
> > +     writel(reg, gphy->u2phy_cfg_base + USBCS_USB2PHY_CFG21_OFFSET);
> > +
> > +     return ret;
> > +}
> > +
> ...
> > +
> > +     gphy->sw = typec_switch_register(dev, &sw_desc);
> > +     if (IS_ERR(gphy->sw))
> > +             return dev_err_probe(dev, PTR_ERR(gphy->sw),
> > +                                  "failed to register typec switch\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static void google_usb_phy_remove(struct platform_device *pdev)
> > +{
> > +     struct google_usb_phy *gphy = dev_get_drvdata(&pdev->dev);
> > +
> > +     typec_switch_unregister(gphy->sw);
> > +     pm_runtime_disable(&pdev->dev);
> > +}
> > +
> > +static const struct of_device_id google_usb_phy_of_match[] = {
> > +     {
> > +             .compatible = "google,gs5-usb-phy",
>
> Did the naming scheme also change from gs{N}01 to gsN?

Starting from Tensor G5, there's no model number [1], hence
the change. The prefix "gs" is kept as their family name.

[1] https://en.wikipedia.org/wiki/Google_Tensor

>
> > +     },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, google_usb_phy_of_match);
> > +
> > +static struct platform_driver google_usb_phy = {
> > +     .probe  = google_usb_phy_probe,
> > +     .remove = google_usb_phy_remove,
> > +     .driver = {
> > +             .name           = "google-usb-phy",
> > +             .of_match_table = google_usb_phy_of_match,
> > +     }
> > +};
> > +
> > +module_platform_driver(google_usb_phy);
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Google USB phy driver");
>
> Great work!
>
> Best regards,
> Ivaylo

Thank you for the review!

Regards,
Roy Luo

Powered by blists - more mailing lists