Re: [PATCH 5/7] phy: qcom: Add M31 based eUSB2 PHY driver

[Wesley Cheng <quic_wcheng@quicinc.com>]

On 1/14/2025 2:34 AM, Dmitry Baryshkov wrote:
> On Mon, Jan 13, 2025 at 01:52:11PM -0800, Melody Olvera wrote:
>> From: Wesley Cheng <quic_wcheng@quicinc.com>
>>
>> On SM8750, the eUSB2 PHY used is M31 based. Add the initialization
>> sequences to bring it out of reset, and to initialize the associated eUSB2
>> repeater as well.
> 
> What does M31 mean? What is the relationship between the eUSB and USB
> M31 PHYs?
> 

M31 is the vendor.  I'll reword this to make it a bit clearer.  There's no
relationship between eUSB2 and USB2 PHY drivers, as the eUSB2 based driver
would require some additional components such as a USB repeater.

>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com>
>> ---
>>  drivers/phy/qualcomm/Kconfig              |  12 +-
>>  drivers/phy/qualcomm/Makefile             |   1 +
>>  drivers/phy/qualcomm/phy-qcom-m31-eusb2.c | 269 ++++++++++++++++++++++++++++++
>>  3 files changed, 281 insertions(+), 1 deletion(-)
> 
> Please run the patch through checkpatch.pl --strict
> 
>>
>> 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..e15529673e358db914936a60fa605c872cd2511a
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-m31-eusb2.c
>> @@ -0,0 +1,269 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2024 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>
>> +
>> +#define USB_PHY_UTMI_CTRL0		(0x3c)
>> +
>> +#define USB_PHY_UTMI_CTRL5		(0x50)
>> +
>> +#define USB_PHY_HS_PHY_CTRL_COMMON0	(0x54)
>> +#define FSEL				(0x7 << 4)
> 
> GENMASK()
> 
>> +#define FSEL_38_4_MHZ_VAL		(0x6 << 4)
> 
> FIELD_PREP
> 
>> +
>> +#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),
>> +};
>> +
>> +static const struct m31_phy_tbl_entry m31_eusb_phy_override_tbl[] = {
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_XCFGI_39_32, GENMASK(3, 2), 0),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_XCFGI_71_64, GENMASK(3, 0), 7),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_XCFGI_31_24, GENMASK(2, 0), 0),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_XCFGI_7_0, GENMASK(1, 0), 0),
>> +};
>> +
>> +static const struct m31_phy_tbl_entry m31_eusb_phy_reset_tbl[] = {
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_HS_PHY_CTRL2, BIT(3), 1),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_HS_PHY_CTRL2, BIT(2), 1),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_UTMI_CTRL0, BIT(0), 1),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_HS_PHY_CTRL_COMMON0, BIT(1), 1),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_HS_PHY_CTRL_COMMON0, BIT(2), 0),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_UTMI_CTRL5, BIT(1), 0),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_HS_PHY_CTRL2, BIT(3), 0),
>> +	M31_EUSB_PHY_INIT_CFG(USB_PHY_CFG0, BIT(1), 0),
>> +};
>> +
>> +struct m31eusb2_phy {
>> +	struct phy			*phy;
>> +	void __iomem			*base;
>> +	const struct m31_eusb2_priv_data	*data;
>> +
>> +	struct regulator		*vreg;
>> +	struct clk			*clk;
>> +	struct reset_control		*reset;
>> +
>> +	struct phy *repeater;
>> +};
>> +
>> +static void msm_m31_eusb2_write_readback(void __iomem *base, u32 offset,
>> +					const u32 mask, u32 val)
>> +{
>> +	u32 write_val, tmp = readl_relaxed(base + offset);
>> +
>> +	tmp &= ~mask;
>> +	write_val = tmp | val;
>> +
>> +	writel_relaxed(write_val, base + offset);
>> +
>> +	tmp = readl_relaxed(base + offset);
>> +	tmp &= mask;
>> +
>> +	if (tmp != val)
>> +		pr_err("write: %x to offset: %x FAILED\n", val, offset);
>> +}
>> +
>> +static void m31eusb2_phy_write_sequence(struct m31eusb2_phy *phy,
>> +					const struct m31_phy_tbl_entry *tbl,
>> +					int num)
>> +{
>> +	int i;
>> +
>> +	for (i = 0 ; i < num; i++, tbl++) {
>> +		dev_dbg(&phy->phy->dev, "Offset:%x BitMask:%x Value:%x",
>> +					tbl->off, tbl->mask, tbl->val);
>> +
>> +		msm_m31_eusb2_write_readback(phy->base,
>> +					tbl->off, tbl->mask,
>> +					tbl->val << __ffs(tbl->mask));
> 
> could you please check, what actually gets written? I suspect there
> should be a -1 here.
> 

The __ffs  uses the ctz/ctzl built in, which counts leading zeros, so the
-1 should already be accounted for.  FIELD_PREP uses the ffs builtin
directly, which would require the -1.  Confirmed that the writes are being
done as expected from the programming tables above.

Thanks
Wesley Cheng