Re: [PATCH v3 06/10] phy: qcom: Add M31 based eUSB2 PHY driver

[Wesley Cheng]

Hi Dmitry,

On 3/26/2025 7:33 AM, Dmitry Baryshkov wrote:
> On Mon, Mar 24, 2025 at 01:18:34PM -0700, Melody Olvera wrote:
> > From: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> >
> > SM8750 utilizes an eUSB2 PHY from M31.  Add the initialization
> > sequences to bring it out of reset and into an operational state.  This
> > differs to the M31 USB driver, in that the M31 eUSB2 driver will
> > require a connection to an eUSB2 repeater.  This PHY driver will handle
> > the initialization of the associated eUSB2 repeater when required.
> >
> > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> > Signed-off-by: Melody Olvera <quic_molvera@xxxxxxxxxxx>
> > ---
> >   drivers/phy/qualcomm/Kconfig              |  10 +
> >   drivers/phy/qualcomm/Makefile             |   1 +
> >   drivers/phy/qualcomm/phy-qcom-m31-eusb2.c | 297 ++++++++++++++++++++++++++++++
> >   3 files changed, 308 insertions(+)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-m31-eusb2.c b/drivers/phy/qualcomm/phy-qcom-m31-eusb2.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..b18aab968122683e2e87287a4b570321d376870a
> > --- /dev/null
> > +++ b/drivers/phy/qualcomm/phy-qcom-m31-eusb2.c
> > @@ -0,0 +1,297 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> GPL-2.0-only

Will fix.

> > +/*
> > + * Copyright (c) 2024-2025 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define USB_PHY_UTMI_CTRL0		(0x3c)
> > +
> > +#define USB_PHY_UTMI_CTRL5		(0x50)
> > +
> > +#define USB_PHY_HS_PHY_CTRL_COMMON0	(0x54)
> > +#define FSEL				GENMASK(6, 4)
> > +#define FSEL_38_4_MHZ_VAL		(0x6)
> > +
> > +#define USB_PHY_HS_PHY_CTRL2		(0x64)
> > +
> > +#define USB_PHY_CFG0			(0x94)
> > +#define USB_PHY_CFG1			(0x154)
> > +
> > +#define USB_PHY_FSEL_SEL		(0xb8)
> > +
> > +#define USB_PHY_XCFGI_39_32		(0x16c)
> > +#define USB_PHY_XCFGI_71_64		(0x17c)
> > +#define USB_PHY_XCFGI_31_24		(0x168)
> > +#define USB_PHY_XCFGI_7_0		(0x15c)
> > +
> > +#define M31_EUSB_PHY_INIT_CFG(o, b, v)	\
> > +{				\
> > +	.off = o,		\
> > +	.mask = b,		\
> > +	.val = v,		\
> > +}
> > +
> > +struct m31_phy_tbl_entry {
> > +	u32 off;
> > +	u32 mask;
> > +	u32 val;
> > +};
> > +
> > +struct m31_eusb2_priv_data {
> > +	const struct m31_phy_tbl_entry	*setup_seq;
> > +	unsigned int			setup_seq_nregs;
> > +	const struct m31_phy_tbl_entry	*override_seq;
> > +	unsigned int			override_seq_nregs;
> > +	const struct m31_phy_tbl_entry	*reset_seq;
> > +	unsigned int			reset_seq_nregs;
> > +	unsigned int			fsel;
> > +};
> > +
> > +static const struct m31_phy_tbl_entry m31_eusb2_setup_tbl[] = {
> > +	M31_EUSB_PHY_INIT_CFG(USB_PHY_CFG0, BIT(1), 1),
> > +	M31_EUSB_PHY_INIT_CFG(USB_PHY_UTMI_CTRL5, BIT(1), 1),
> > +	M31_EUSB_PHY_INIT_CFG(USB_PHY_CFG1, BIT(0), 1),
> > +	M31_EUSB_PHY_INIT_CFG(USB_PHY_FSEL_SEL, BIT(0), 1),
>
> I suppose, we can not expect to have #defines for all used bitfields?

Added the bitfields as requested.

> > +};
> > +
>
> [...]
>
> > +
> > +static const struct phy_ops m31eusb2_phy_gen_ops = {
> > +	.init	= m31eusb2_phy_init,
> > +	.exit	= m31eusb2_phy_exit,
>
> I think, you have a missing .set_mode callback here.

Added this to properly set the mode on the repeater as well.

> > +	.owner	= THIS_MODULE,
> > +};
> > +
> > +static int m31eusb2_phy_probe(struct platform_device *pdev)
> > +{
> > +	struct phy_provider *phy_provider;
> > +	const struct m31_eusb2_priv_data *data;
> > +	struct device *dev = &pdev->dev;
> > +	struct m31eusb2_phy *phy;
> > +	int ret;
> > +
> > +	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
> > +	if (!phy)
> > +		return -ENOMEM;
> > +
> > +	data = of_device_get_match_data(dev);
>
> device_get_match_data()

Done.

Thanks
Wesley Cheng