Re: [PATCH v1 2/3] usb: phy: Add driver for the Realtek SoC USB 2.0/3.0 PHY

[Arnd Bergmann]

On Fri, May 19, 2023, at 06:58, Stanley Chang wrote:
> +struct rtk_usb_phy {
> +	struct usb_phy phy;
> +	struct device *dev;
> +	struct regmap *usb_regs;
> +	struct regmap *mac_regs;
> +	struct regmap *usb_ctrl_regs;
> +
> +	int port_index;
> +	int phyN;
> +	void *reg_addr;
> +	void *phy_data;
> +
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry *debug_dir;
> +#endif
> +};

I'd avoid the #ifdefs here and just leave the debugfs
code in unconditionally in favor of readability. When
debugfs is disabled, everything except for the one pointer
value should get optimized out.

> +#define phy_read(addr) __raw_readl(addr)
> +#define phy_write(addr, val) do { \
> +	/* Do smp_wmb */ \
> +	smp_wmb(); __raw_writel(val, addr); \
> +} while (0)

Using __raw_readl()/__raw_writel() in a driver is almost never
the right interface, it does not guarantee atomicity of the
access, has the wrong byte-order on big-endian kernels and misses
the barriers to serialize against DMA accesses. smp_wmb()
should have no effect here since this only serializes access to
memory against another CPU if it's paired with an smp_rmb(), but
not on MMIO registers.

> +#define PHY_IO_TIMEOUT_MSEC		(50)
> +
> +static inline int utmi_wait_register(void __iomem *reg, u32 mask, u32 
> result)
> +{
> +	unsigned long timeout = jiffies + 
> msecs_to_jiffies(PHY_IO_TIMEOUT_MSEC);
> +
> +	while (time_before(jiffies, timeout)) {
> +		/* Do smp_rmb */
> +		smp_rmb();
> +		if ((phy_read(reg) & mask) == result)
> +			return 0;
> +		udelay(100);
> +	}
> +	pr_err("\033[0;32;31m can't program USB phy \033[m\n");
> +
> +	return -ETIMEDOUT;
> +}

This should just use read_poll_timeout() or possibly
read_poll_timeout_atomic(), but not an open-coded version.

I don't think I've seen escape sequences in a printk
in any other driver, so please don't start that now.

> +#define DEFAULT_CHIP_REVISION 0xA00
> +#define MAX_CHIP_REVISION 0xC00
> +
> +static inline int __get_chip_revision(void)
> +{
> +	int chip_revision = 0xFFF;
> +	char revision[] = "FFF";
> +	struct soc_device_attribute soc_att[] = {{.revision = revision}, {}};

You should probably check that you are actually on the right
SoC type here as well, not just the right revision of
an arbitrary chip.

Ideally I think the revision check should be based off a DT proporty
if that's possible, so you can have this code in the boot loader.

> +#define RTK_USB2PHY_NAME "rtk-usb2phy"

Better avoid hiding the driver name like this, it makes it harder
to grep the source tree for particular driver names.

> +	/* rmb for reg read */
> +	smp_rmb();
> +	regVal = phy_read(reg_gusb2phyacc0);

I would expect that you don't need barriers like this, especially
if you change the phy_read() helper to use the proper readl().

If you do need to serialize against other CPUs, still, there should
be a longer explanation about that, since it's so unexpected.

> +
> +static void do_rtk_usb2_phy_toggle(struct rtk_usb_phy *rtk_phy,
> +	    int index, bool isConnect);

It's best to sort your function definitions in a way that avoids
forward declarations. This makes it easier to read and shows that
there are no obvious recursions in the source. If you do have
an intentional recursion, make sure that there is a way to
prevent unbounded stack usage, and explain that in a comment.

> +static int do_rtk_usb_phy_init(struct rtk_usb_phy *rtk_phy, int index)
> +{
> +	struct reg_addr *regAddr;
> +	struct phy_data *phy_data;
> +	struct phy_parameter *phy_page_setting;
> +	int i;
> +
> +	if (!rtk_phy) {
> +		pr_err("%s, rtk_phy is NULL\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(rtk_phy->dev, "%s: init phy#%d\n", __func__, index);
...
> +	if (!phy_data) {
> +		pr_err("%s, phy_data is NULL\n", __func__);
> +		return -EINVAL;
> +	}

You can probably remove most of the debugging prints.

> +	regAddr = &((struct reg_addr *)rtk_phy->reg_addr)[index];
> +	phy_data = &((struct phy_data *)rtk_phy->phy_data)[index];

Why do you need the casts here? It looks like regAddr should
be an __iomem pointer. Please build your driver with 'make C=1'
to see if there are any incorrect address space annotations.

> +static int __get_phy_parameter_by_efuse(struct rtk_usb_phy *rtk_phy,
> +	    struct phy_data *phy_data, int index)
> +{
> +	u8 value = 0;
> +	struct nvmem_cell *cell;
> +	struct soc_device_attribute rtk_soc_groot[] = {
> +			{ .family = "Realtek Groot",},
> +			{ /* empty */ }
> +		};
> +	struct soc_device_attribute rtk_soc_hank[] = {
> +			{ .family = "Realtek Hank",},
> +			{ /* empty */ }
> +		};
> +	struct soc_device_attribute rtk_soc_efuse_v1[] = {
> +			{ .family = "Realtek Phoenix",},
> +			{ .family = "Realtek Kylin",},
> +			{ .family = "Realtek Hercules",},
> +			{ .family = "Realtek Thor",},
> +			{ .family = "Realtek Hank",},
> +			{ .family = "Realtek Groot",},
> +			{ .family = "Realtek Stark",},
> +			{ .family = "Realtek Parker",},
> +			{ /* empty */ }
> +		};
> +	struct soc_device_attribute rtk_soc_dis_level_at_page0[] = {
> +			{ .family = "Realtek Phoenix",},
> +			{ .family = "Realtek Kylin",},
> +			{ .family = "Realtek Hercules",},
> +			{ .family = "Realtek Thor",},
> +			{ .family = "Realtek Hank",},
> +			{ .family = "Realtek Groot",},
> +			{ /* empty */ }
> +		};
> +
> +	if (soc_device_match(rtk_soc_efuse_v1)) {
> +		dev_dbg(rtk_phy->dev, "Use efuse v1 to updated phy parameter\n");
> +		phy_data->check_efuse_version = CHECK_EFUSE_V1;

I'm not entirely sure what you are trying to do here, but
it looks the purpose is to tell the difference between implementations
of the phy device by looking at which SoC it's in. You should
only need that very rarely when this information cannot be
passed through the DT, but you literally already have the
per-SoC compatible strings below, so just use those, or add other
DT properties in the binding for specific quirks or capabilities.

> +#ifdef CONFIG_OF
> +static const struct of_device_id usbphy_rtk_dt_match[] = {
> +	{ .compatible = "realtek,usb3phy", },
> +	{ .compatible = "realtek,rtd-usb3phy", },
> +	{ .compatible = "realtek,rtd1295-usb3phy", },
> +	{ .compatible = "realtek,rtd1619-usb3phy", },
> +	{ .compatible = "realtek,rtd1319-usb3phy", },
> +	{ .compatible = "realtek,rtd1619b-usb3phy", },
> +	{ .compatible = "realtek,rtd1319d-usb3phy", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, usbphy_rtk_dt_match);
> +#endif
> +
> +static struct platform_driver rtk_usb3phy_driver = {
> +	.probe		= rtk_usb3phy_probe,
> +	.remove		= rtk_usb3phy_remove,
> +	.driver		= {
> +		.name	= RTK_USB3PHY_NAME,
> +		.owner	= THIS_MODULE,
> +		.of_match_table = of_match_ptr(usbphy_rtk_dt_match),
> +	},
> +};

Remove that of_match_ptr() and ifdef CONFIG_OF check here, new drivers
should no longer use static platform device definitions and just assume
that CONFIG_OF is used.

     Arnd