Re: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

Re: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

[kernel test robot]

Hi Stanley,

kernel test robot noticed the following build errors:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus robh/for-next linus/master v6.4-rc5 next-20230607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanley-Chang/phy-realtek-usb-Add-driver-for-the-Realtek-SoC-USB-2-0-PHY/20230607-142704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230607062500.24669-2-stanley_chang%40realtek.com
patch subject: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY
config: sh-randconfig-r033-20230607 (https://download.01.org/0day-ci/archive/20230607/202306071901.mhcH1Kcc-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add usb https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
        git fetch usb usb-testing
        git checkout usb/usb-testing
        b4 shazam https://lore.kernel.org/r/20230607062500.24669-2-stanley_chang@realtek.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir --shuffle=2827500481 ARCH=sh olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir --shuffle=2827500481 ARCH=sh SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306071901.mhcH1Kcc-lkp@intel.com/

All errors (new ones prefixed by >>):

   sh4-linux-ld: drivers/phy/realtek/phy-rtk-usb2.o: in function `rtk_usb2phy_remove':
>> drivers/phy/realtek/phy-rtk-usb2.c:1891: undefined reference to `usb_remove_phy'
   sh4-linux-ld: drivers/phy/realtek/phy-rtk-usb2.o: in function `create_debug_files':
>> drivers/phy/realtek/phy-rtk-usb2.c:1378: undefined reference to `usb_debug_root'
   sh4-linux-ld: drivers/phy/realtek/phy-rtk-usb2.o: in function `rtk_usb2phy_probe':
>> drivers/phy/realtek/phy-rtk-usb2.c:1882: undefined reference to `usb_add_phy_dev'
   sh4-linux-ld: drivers/power/supply/wm831x_power.o: in function `wm831x_power_probe':
>> drivers/power/supply/wm831x_power.c:695: undefined reference to `devm_usb_get_phy_by_phandle'
   sh4-linux-ld: drivers/power/supply/rt9455_charger.o: in function `rt9455_probe':
>> drivers/power/supply/rt9455_charger.c:1682: undefined reference to `devm_usb_get_phy'
   sh4-linux-ld: drivers/power/supply/isp1704_charger.o: in function `isp1704_charger_probe':
>> drivers/power/supply/isp1704_charger.c:73: undefined reference to `devm_usb_get_phy_by_phandle'
>> sh4-linux-ld: drivers/power/supply/isp1704_charger.c:73: undefined reference to `devm_usb_get_phy'
   sh4-linux-ld: drivers/power/supply/bq25890_charger.o: in function `bq25890_probe':
>> drivers/power/supply/bq25890_charger.c:1511: undefined reference to `devm_usb_get_phy'
   sh4-linux-ld: drivers/power/supply/bq256xx_charger.o: in function `bq256xx_probe':
>> drivers/power/supply/bq256xx_charger.c:1743: undefined reference to `devm_usb_get_phy'

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for USB_PHY
   Depends on [n]: USB_SUPPORT [=n]
   Selected by [y]:
   - PHY_RTK_RTD_USB2PHY [=y]


vim +1891 drivers/phy/realtek/phy-rtk-usb2.c

  1802	
  1803	static int rtk_usb2phy_probe(struct platform_device *pdev)
  1804	{
  1805		struct rtk_usb_phy *rtk_phy;
  1806		struct device *dev = &pdev->dev;
  1807		struct device_node *node;
  1808		struct device_node *sub_node;
  1809		struct phy *generic_phy;
  1810		struct phy_provider *phy_provider;
  1811		int phyN, ret = 0;
  1812	
  1813		rtk_phy = devm_kzalloc(dev, sizeof(*rtk_phy), GFP_KERNEL);
  1814		if (!rtk_phy)
  1815			return -ENOMEM;
  1816	
  1817		rtk_phy->dev			= &pdev->dev;
  1818		rtk_phy->phy.dev		= rtk_phy->dev;
  1819		rtk_phy->phy.label		= "rtk-usb2phy";
  1820		rtk_phy->phy.notify_port_status = rtk_usb_phy_notify_port_status;
  1821	
  1822		if (!dev->of_node) {
  1823			dev_err(dev, "%s %d No device node\n", __func__, __LINE__);
  1824			goto err;
  1825		}
  1826	
  1827		node = dev->of_node;
  1828	
  1829		rtk_phy->usb_ctrl_regs = syscon_regmap_lookup_by_phandle(node, "realtek,usb-ctrl");
  1830		if (IS_ERR(rtk_phy->usb_ctrl_regs)) {
  1831			dev_info(dev, "%s: DTS no support usb_ctrl regs syscon\n", __func__);
  1832			rtk_phy->usb_ctrl_regs = NULL;
  1833		}
  1834	
  1835		phyN = of_get_child_count(node);
  1836		rtk_phy->phyN = phyN;
  1837		dev_dbg(dev, "%s phyN=%d\n", __func__, rtk_phy->phyN);
  1838	
  1839		rtk_phy->reg_addr = devm_kzalloc(dev,
  1840			    sizeof(struct reg_addr) * phyN, GFP_KERNEL);
  1841		if (!rtk_phy->reg_addr)
  1842			return -ENOMEM;
  1843	
  1844		rtk_phy->phy_data = devm_kzalloc(dev,
  1845			    sizeof(struct phy_data) * phyN, GFP_KERNEL);
  1846		if (!rtk_phy->phy_data)
  1847			return -ENOMEM;
  1848	
  1849		for (sub_node = of_get_next_child(node, NULL); sub_node != NULL;
  1850			    sub_node = of_get_next_child(node, sub_node)) {
  1851			ret = get_phy_parameter(rtk_phy, sub_node);
  1852			if (ret) {
  1853				dev_err(dev, "%s: get_phy_parameter fail ret=%d\n",
  1854					    __func__, ret);
  1855				goto err;
  1856			}
  1857		}
  1858	
  1859		platform_set_drvdata(pdev, rtk_phy);
  1860	
  1861		generic_phy = devm_phy_create(rtk_phy->dev, NULL, &ops);
  1862		if (IS_ERR(generic_phy))
  1863			return PTR_ERR(generic_phy);
  1864	
  1865		phy_set_drvdata(generic_phy, rtk_phy);
  1866	
  1867		phy_provider = devm_of_phy_provider_register(rtk_phy->dev,
  1868					    of_phy_simple_xlate);
  1869		if (IS_ERR(phy_provider))
  1870			return PTR_ERR(phy_provider);
  1871	
  1872		ret = usb_add_phy_dev(&rtk_phy->phy);
  1873		if (ret)
  1874			goto err;
  1875	
  1876		create_debug_files(rtk_phy);
  1877	
  1878	err:
  1879		dev_dbg(dev, "Probe RTK USB 2.0 PHY (ret=%d)\n", ret);
  1880	
  1881		return ret;
> 1882	}
  1883	
  1884	static void rtk_usb2phy_remove(struct platform_device *pdev)
  1885	{
  1886		struct rtk_usb_phy *rtk_phy = platform_get_drvdata(pdev);
  1887	
  1888		remove_debug_files(rtk_phy);
  1889	
  1890		usb_remove_phy(&rtk_phy->phy);
> 1891	}
  1892	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY

[Krzysztof Kozlowski]

On 07/06/2023 08:24, Stanley Chang wrote:
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the USB 3.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---


> +static inline int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
> +{
> +	int ret;
> +	unsigned int val;
> +
> +	ret = read_poll_timeout(readl, val, ((val & mask) == result),
> +		    PHY_IO_DELAY_US, PHY_IO_TIMEOUT_USEC, false, reg);
> +	if (ret) {
> +		pr_err("%s can't program USB phy\n", __func__);
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rtk_usb_phy3_wait_vbusy(struct reg_addr *regAddr)
> +{
> +	return utmi_wait_register(regAddr->reg_mdio_ctl, USB_MDIO_CTRL_PHY_BUSY, 0);
> +}
> +
> +static u16 rtk_usb_phy_read(struct reg_addr *regAddr, char addr)
> +{
> +	unsigned int regVal;
> +	u32 value;
> +
> +	regVal = (addr << USB_MDIO_CTRL_PHY_ADDR_SHIFT);
> +
> +	writel(regVal, regAddr->reg_mdio_ctl);
> +
> +	rtk_usb_phy3_wait_vbusy(regAddr);
> +
> +	value = readl(regAddr->reg_mdio_ctl);
> +	value = value >> USB_MDIO_CTRL_PHY_DATA_SHIFT;
> +
> +	return (u16)value;
> +}
> +
> +static int rtk_usb_phy_write(struct reg_addr *regAddr, char addr, u16 data)
> +{
> +	unsigned int regVal;
> +
> +	regVal = USB_MDIO_CTRL_PHY_WRITE |
> +		    (addr << USB_MDIO_CTRL_PHY_ADDR_SHIFT) |
> +		    (data << USB_MDIO_CTRL_PHY_DATA_SHIFT);
> +
> +	writel(regVal, regAddr->reg_mdio_ctl);
> +
> +	rtk_usb_phy3_wait_vbusy(regAddr);
> +
> +	return 0;
> +}
> +
> +static void do_rtk_usb3_phy_toggle(struct rtk_usb_phy *rtk_phy, int i,
> +	    bool isConnect)
> +{
> +	struct reg_addr *regAddr;
> +	struct phy_data *phy_data;
> +	struct phy_parameter *phy_parameter;
> +	size_t index;
> +
> +	regAddr = &((struct reg_addr *)rtk_phy->reg_addr)[i];
> +	phy_data = &((struct phy_data *)rtk_phy->phy_data)[i];
> +
> +	if (!phy_data) {
> +		dev_err(rtk_phy->dev, "%s phy_data is NULL!\n", __func__);

???

> +		return;
> +	}
> +
> +	if (!phy_data->do_toggle)
> +		return;
> +
> +	phy_parameter = phy_data->parameter;
> +
> +	index = PHY_ADDR_MAP_ARRAY_INDEX(PHY_ADDR_0x09);
> +
> +	if (index < phy_data->size) {
> +		u8 addr = (phy_parameter + index)->addr;
> +		u16 data = (phy_parameter + index)->data;
> +
> +		if (addr == 0xFF) {
> +			addr = ARRAY_INDEX_MAP_PHY_ADDR(index);
> +			data = rtk_usb_phy_read(regAddr, addr);
> +			(phy_parameter + index)->addr = addr;
> +			(phy_parameter + index)->data = data;
> +		}
> +		mdelay(1);
> +		dev_info(rtk_phy->dev,
> +			    "%s ########## to toggle PHY addr 0x09 BIT(9)\n",
> +			    __func__);
> +		rtk_usb_phy_write(regAddr, addr, data&(~BIT(9)));
> +		mdelay(1);
> +		rtk_usb_phy_write(regAddr, addr, data);
> +	}
> +	dev_info(rtk_phy->dev, "%s ########## PHY addr 0x1f = 0x%04x\n",
> +		    __func__, rtk_usb_phy_read(regAddr, PHY_ADDR_0x1F));

Please drop all simple debug success messages. Linux has already
infrastructure for this.


...

> +	return 0;
> +}
> +
> +static int rtk_usb_phy_init(struct phy *phy)
> +{
> +	struct rtk_usb_phy *rtk_phy = phy_get_drvdata(phy);
> +	int ret = 0;
> +	int i;
> +	unsigned long phy_init_time = jiffies;
> +
> +	if (!rtk_phy) {
> +		pr_err("%s rtk_phy is NULL!\n", __func__);

What? How is this possible?

> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(rtk_phy->dev, "Init RTK USB 3.0 PHY\n");
> +	for (i = 0; i < rtk_phy->phyN; i++)
> +		ret = do_rtk_usb_phy_init(rtk_phy, i);
> +
> +	dev_info(rtk_phy->dev, "Initialized RTK USB 3.0 PHY (take %dms)\n",
> +		    jiffies_to_msecs(jiffies - phy_init_time));

Please drop all simple debug success messages. Linux has already
infrastructure for this.

> +	return ret;
> +}
> +
> +static int rtk_usb_phy_exit(struct phy *phy)
> +{
> +	struct rtk_usb_phy *rtk_phy = phy_get_drvdata(phy);
> +
> +	if (!rtk_phy) {
> +		pr_err("%s rtk_phy is NULL!\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(rtk_phy->dev, "Exit RTK USB 3.0 PHY\n");

Please drop all simple debug success messages. Linux has already
infrastructure for this.

> +static void rtk_usb_phy_toggle(struct usb_phy *usb3_phy, bool isConnect, int port)
> +{
> +	int index = port;
> +	struct rtk_usb_phy *rtk_phy = NULL;
> +
> +	if (usb3_phy != NULL && usb3_phy->dev != NULL)
> +		rtk_phy = dev_get_drvdata(usb3_phy->dev);
> +
> +	if (rtk_phy == NULL) {
> +		pr_err("%s ERROR! NO this device\n", __func__);

Your error messages are not helping. No need to shout, no need to handle
some non-existing cases. If this is real case, you have broken driver. I
actually suspect that.

How can you interface with a driver where there is no device?

> +		return;
> +	}
> +
> +	if (index > rtk_phy->phyN) {
> +		pr_err("%s %d ERROR! port=%d > phyN=%d\n",
> +			    __func__, __LINE__, index, rtk_phy->phyN);
> +		return;
> +	}
> +
> +	do_rtk_usb3_phy_toggle(rtk_phy, index, isConnect);
> +}
> +
> +static int rtk_usb_phy_notify_port_status(struct usb_phy *x, int port,
> +	    u16 portstatus, u16 portchange)
> +{
> +	bool isConnect = false;

This is not C++. Don't use camelcase. See Coding style document.

> +
> +	pr_debug("%s port=%d portstatus=0x%x portchange=0x%x\n",
> +		    __func__, port, (int)portstatus, (int)portchange);
> +	if (portstatus & USB_PORT_STAT_CONNECTION)
> +		isConnect = true;
> +

...

> +
> +static int rtk_usb3_set_parameter_show(struct seq_file *s, void *unused)
> +{
> +	struct rtk_usb_phy *rtk_phy = s->private;
> +	const struct file *file = s->file;
> +	const char *file_name = file_dentry(file)->d_iname;
> +	struct dentry *p_dentry = file_dentry(file)->d_parent;
> +	const char *phy_dir_name = p_dentry->d_iname;
> +	int ret, index;
> +	struct phy_data *phy_data = NULL;
> +
> +	for (index = 0; index < rtk_phy->phyN; index++) {
> +		size_t sz = 30;
> +		char name[30] = {0};
> +
> +		snprintf(name, sz, "phy%d", index);
> +		if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> +			phy_data = &((struct phy_data *)rtk_phy->phy_data)[index];
> +			break;
> +		}
> +	}
> +	if (!phy_data) {
> +		dev_err(rtk_phy->dev,
> +				    "%s: No phy_data for %s/%s\n",
> +				    __func__, phy_dir_name, file_name);

Mess wrapping/indentation. Actually everywhere in the file...

> +		return -EINVAL;
> +	}
> +
> +	ret = __get_parameter_at_page(s, rtk_phy, phy_data->parameter, file_name);
> +	if (ret < 0)
> +		return ret;
> +
> +	seq_puts(s, "Set phy parameter by following command\n");
> +	seq_printf(s, "echo \"value\" > %s/%s\n",
> +		    phy_dir_name, file_name);
> +
> +	return 0;
> +}
> +
> +static int rtk_usb3_set_parameter_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, rtk_usb3_set_parameter_show, inode->i_private);
> +}
> +
> +static ssize_t rtk_usb3_set_parameter_write(struct file *file,
> +		const char __user *ubuf, size_t count, loff_t *ppos)
> +{
> +	const char *file_name = file_dentry(file)->d_iname;
> +	struct dentry *p_dentry = file_dentry(file)->d_parent;
> +	const char *phy_dir_name = p_dentry->d_iname;
> +	struct seq_file		*s = file->private_data;
> +	struct rtk_usb_phy		*rtk_phy = s->private;
> +	struct reg_addr *regAddr = NULL;
> +	struct phy_data *phy_data = NULL;
> +	int ret = 0;
> +	char buffer[40] = {0};
> +	int index;
> +
> +	if (copy_from_user(&buffer, ubuf,
> +		    min_t(size_t, sizeof(buffer) - 1, count)))
> +		return -EFAULT;
> +
> +	for (index = 0; index < rtk_phy->phyN; index++) {
> +		size_t sz = 30;
> +		char name[30] = {0};
> +
> +		snprintf(name, sz, "phy%d", index);
> +		if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> +			regAddr = &((struct reg_addr *)rtk_phy->reg_addr)[index];
> +			phy_data = &((struct phy_data *)rtk_phy->phy_data)[index];
> +			break;


Where is the ABI documentation for user interface?


> +
> +static inline void create_debug_files(struct rtk_usb_phy *rtk_phy)
> +{
> +	struct dentry *phy_debug_root = NULL;
> +	struct dentry *set_parameter_dir = NULL;
> +
> +	phy_debug_root = create_phy_debug_root();
> +
> +	if (!phy_debug_root) {
> +		dev_err(rtk_phy->dev, "%s Error phy_debug_root is NULL",
> +			    __func__);
> +		return;
> +	}
> +	rtk_phy->debug_dir = debugfs_create_dir(dev_name(rtk_phy->dev),
> +		    phy_debug_root);
> +	if (!rtk_phy->debug_dir) {
> +		dev_err(rtk_phy->dev, "%s Error debug_dir is NULL", __func__);

Are you sure you run checkpatch on this? Error messages on debugfs are
almost always incorrect.

> +		return;
> +	}
> +
> +	if (!debugfs_create_file("parameter", 0444,
> +		    rtk_phy->debug_dir, rtk_phy,
> +		    &rtk_usb3_parameter_fops))
> +		goto file_error;
> +
> +	set_parameter_dir = debugfs_create_dir("set_parameter",
> +		    rtk_phy->debug_dir);
> +	if (set_parameter_dir) {
> +		int index, ret;
> +
> +		for (index = 0; index < rtk_phy->phyN; index++) {
> +			struct dentry *phy_dir;
> +			struct phy_data *phy_data;
> +			size_t sz = 30;
> +			char name[30] = {0};
> +
> +			snprintf(name, sz, "phy%d", index);
> +
> +			phy_data = &((struct phy_data *)rtk_phy->phy_data)[index];
> +
> +			phy_dir = debugfs_create_dir(name, set_parameter_dir);
> +			if (!phy_dir) {
> +				dev_err(rtk_phy->dev,
> +					    "%s Error create folder %s fail\n",
> +					    name, __func__);
> +				goto file_error;
> +			}
> +
> +			ret = create_debug_set_parameter_files(rtk_phy, phy_dir,
> +				    phy_data->size);
> +			if (ret < 0) {
> +				dev_err(rtk_phy->dev,
> +					    "%s Error create files fail\n",
> +					    __func__);
> +				goto file_error;
> +			}
> +		}
> +	}
> +
> +	if (!debugfs_create_file("toggle", 0644, rtk_phy->debug_dir, rtk_phy,
> +		    &rtk_usb3_toggle_fops))
> +		goto file_error;
> +
> +	return;
> +
> +file_error:
> +	debugfs_remove_recursive(rtk_phy->debug_dir);
> +}
> +
> +static inline void remove_debug_files(struct rtk_usb_phy *rtk_phy)
> +{
> +	debugfs_remove_recursive(rtk_phy->debug_dir);
> +}
> +#else
> +static inline void create_debug_files(struct rtk_usb_phy *rtk_phy) { }
> +static inline void remove_debug_files(struct rtk_usb_phy *rtk_phy) { }
> +#endif /* CONFIG_DEBUG_FS */
> +
> +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;
> +
> +	if (!phy_data->check_efuse)
> +		goto out;
> +
> +	cell = nvmem_cell_get(rtk_phy->dev, "usb_u3_tx_lfps_swing_trim");
> +	if (IS_ERR(cell)) {
> +		dev_warn(rtk_phy->dev,
> +			    "%s failed to get usb_u3_tx_lfps_swing_trim: %ld\n",
> +			    __func__, PTR_ERR(cell));
> +	} else {
> +		unsigned char *buf;
> +		size_t buf_size;
> +
> +		buf = nvmem_cell_read(cell, &buf_size);
> +
> +		value = buf[0] & USB_U3_TX_LFPS_SWING_TRIM_MASK;
> +
> +		dev_dbg(rtk_phy->dev,
> +			    "phy index=%d buf=0x%x buf_size=%d value=0x%x\n",
> +			    index, buf[0], (int)buf_size, value);
> +		kfree(buf);
> +		nvmem_cell_put(cell);
> +	}
> +
> +	if ((value > 0) && (value < 0x8))
> +		phy_data->efuse_usb_u3_tx_lfps_swing_trim = 0x8;
> +	else
> +		phy_data->efuse_usb_u3_tx_lfps_swing_trim = (u8)value;
> +
> +	dev_dbg(rtk_phy->dev, "Get Efuse usb_u3_tx_lfps_swing_trim=0x%x (value=0x%x)\n",
> +		    phy_data->efuse_usb_u3_tx_lfps_swing_trim, value);
> +
> +out:
> +	return 0;
> +}
> +
> +static int __get_phy_parameter(struct device *dev, struct phy_data *phy_data,
> +	    struct device_node *sub_node)
> +{
> +	struct phy_parameter *phy_parameter;
> +	int i, ret = 0;
> +	int data_size, num_cells = 2;
> +
> +	phy_data->size = MAX_USB_PHY_DATA_SIZE;
> +	phy_data->parameter = devm_kzalloc(dev,
> +		    sizeof(struct phy_parameter) * phy_data->size, GFP_KERNEL);
> +	if (!phy_data->parameter)
> +		return -ENOMEM;
> +
> +	if (!of_get_property(sub_node, "realtek,param", &data_size)) {
> +		dev_dbg(dev, "%s No parameter (data_size=%d)\n",
> +			    __func__, data_size);
> +		data_size = 0;
> +	}
> +
> +	if (!data_size)
> +		goto out;
> +
> +	phy_parameter = phy_data->parameter;
> +	/* Set default addr to 0xff for no data case */
> +	for (i = 0; i < phy_data->size; i++)
> +		(phy_parameter + i)->addr = 0xFF;
> +
> +	data_size = data_size / (sizeof(u32) * num_cells);
> +	for (i = 0; i < data_size; i++) {
> +		struct phy_parameter *parameter;
> +		u32 addr, data;
> +		int offset, index;
> +
> +		offset = i * num_cells;
> +
> +		ret = of_property_read_u32_index(sub_node, "realtek,param",
> +			    offset, &addr);
> +		if (ret) {
> +			dev_err(dev, "ERROR: To get param i=%d addr=0x%x\n",
> +				    i, addr);
> +			break;
> +		}
> +
> +		ret = of_property_read_u32_index(sub_node, "realtek,param",
> +			    offset + 1, &data);
> +		if (ret) {
> +			dev_err(dev, "ERROR: To get param i=%d addr=0x%x\n",
> +				    i, data);
> +			break;
> +		}
> +
> +		index = PHY_ADDR_MAP_ARRAY_INDEX(addr);
> +		parameter = (phy_parameter + index);
> +		parameter->addr = (u8)addr;
> +		parameter->data = (u16)data;
> +
> +		dev_dbg(dev, "param index=%d addr=0x%x data=0x%x\n", index,
> +			    parameter->addr, parameter->data);
> +	}
> +
> +out:
> +	return ret;
> +}
> +
> +static int get_phy_parameter(struct rtk_usb_phy *rtk_phy,
> +	    struct device_node *sub_node)
> +{
> +	struct device *dev = rtk_phy->dev;
> +	struct reg_addr *addr;
> +	struct phy_data *phy_data;
> +	int ret = 0;
> +	int index;
> +
> +	if (of_property_read_u32(sub_node, "reg", &index)) {
> +		dev_err(dev, "sub_node without reg\n");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "sub_node index=%d\n", index);

Please drop all simple debug success messages. Linux has already
infrastructure for this.

...

> +
> +static int rtk_usb3phy_probe(struct platform_device *pdev)
> +{
> +	struct rtk_usb_phy *rtk_phy;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node;
> +	struct device_node *sub_node;
> +	struct phy *generic_phy;
> +	struct phy_provider *phy_provider;
> +	int ret, phyN;
> +
> +	rtk_phy = devm_kzalloc(dev, sizeof(*rtk_phy), GFP_KERNEL);
> +	if (!rtk_phy)
> +		return -ENOMEM;
> +
> +	rtk_phy->dev			= &pdev->dev;
> +	rtk_phy->phy.dev		= rtk_phy->dev;
> +	rtk_phy->phy.label		= "rtk-usb3phy";
> +	rtk_phy->phy.notify_port_status = rtk_usb_phy_notify_port_status;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "%s %d No device node\n", __func__, __LINE__);

How is it even possible? If you do not have device node here, how did it
probe?

> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	node = dev->of_node;
> +
> +	phyN = of_get_child_count(node);
> +	rtk_phy->phyN = phyN;
> +	dev_dbg(dev, "%s phyN=%d\n", __func__, rtk_phy->phyN);

Please drop all simple debug success messages. Linux has already
infrastructure for this.

> +
> +	rtk_phy->reg_addr = devm_kzalloc(dev,
> +		    sizeof(struct reg_addr) * phyN, GFP_KERNEL);
> +	if (!rtk_phy->reg_addr)
> +		return -ENOMEM;
> +
> +	rtk_phy->phy_data = devm_kzalloc(dev,
> +		    sizeof(struct phy_data) * phyN, GFP_KERNEL);
> +	if (!rtk_phy->phy_data)
> +		return -ENOMEM;
> +
> +	for (sub_node = of_get_next_child(node, NULL); sub_node != NULL;
> +		    sub_node = of_get_next_child(node, sub_node)) {
> +		ret = get_phy_parameter(rtk_phy, sub_node);
> +		if (ret) {
> +			dev_err(dev, "%s: get_phy_parameter fail ret=%d\n",
> +				    __func__, ret);
> +			goto err;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, rtk_phy);
> +
> +	generic_phy = devm_phy_create(rtk_phy->dev, NULL, &ops);
> +	if (IS_ERR(generic_phy))
> +		return PTR_ERR(generic_phy);
> +
> +	phy_set_drvdata(generic_phy, rtk_phy);
> +
> +	phy_provider = devm_of_phy_provider_register(rtk_phy->dev,
> +				    of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	ret = usb_add_phy_dev(&rtk_phy->phy);
> +	if (ret)
> +		goto err;
> +
> +	create_debug_files(rtk_phy);
> +
> +err:
> +	dev_dbg(&pdev->dev, "Probe RTK USB 3.0 PHY (ret=%d)\n", ret);

Please drop all simple debug success messages. Linux has already
infrastructure for this.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 07/06/2023 08:24, Stanley Chang wrote:
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the USB 2.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> v2 to v3 change:
>     1. Broken down into two patches, one for each of USB 2 & 3 PHY.
>     2. Removed parameter v1 support for simplification.
>     3. Use remove_new for driver remove callback.


...

> +	platform_set_drvdata(pdev, rtk_phy);
> +
> +	generic_phy = devm_phy_create(rtk_phy->dev, NULL, &ops);
> +	if (IS_ERR(generic_phy))
> +		return PTR_ERR(generic_phy);
> +
> +	phy_set_drvdata(generic_phy, rtk_phy);
> +
> +	phy_provider = devm_of_phy_provider_register(rtk_phy->dev,
> +				    of_phy_simple_xlate);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	ret = usb_add_phy_dev(&rtk_phy->phy);
> +	if (ret)
> +		goto err;
> +
> +	create_debug_files(rtk_phy);
> +
> +err:
> +	dev_dbg(dev, "Probe RTK USB 2.0 PHY (ret=%d)\n", ret);

I commented on your second patch, but everything is applicable here as
well. You have many useless debug messages. Many incorrect or useless
"if() return" which point to broken driver design (e.g. concurrent
access to half initialized structures where you substitute lack of
synchronization with incorrect "if() return"). Undocumented user
interface is one more big trouble.

I doubt you run checkpatch on this (be sure to run it with --strict and
fix almost everything).


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 4/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 07/06/2023 08:24, Stanley Chang wrote:
> Add the documentation explain the property about Realtek USB PHY driver.
> 
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the USB 2.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> v2 to v3 change:
>     1. Broken down into two patches, one for each of USB 2 & 3.
>     2. Add more description about Realtek RTD SoCs architecture.
>     3. Removed parameter v1 support for simplification.
>     4. Revised the compatible name for fallback compatible.
>     5. Remove some properties that can be set in the driver.
> v1 to v2 change:
>     Add phy-cells for generic phy driver
> ---
>  .../bindings/phy/realtek,usb2phy.yaml         | 213 ++++++++++++++++++
>  1 file changed, 213 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml b/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> new file mode 100644
> index 000000000000..69911e20a561
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,usb2phy.yaml
> @@ -0,0 +1,213 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Realtek Semiconductor Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,usb2phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC SoCs USB 2.0 PHY
> +
> +maintainers:
> +  - Stanley Chang <stanley_chang@realtek.com>
> +
> +description:
> +  Realtek USB 2.0 PHY support the digital home center (DHC) RTD series SoCs.
> +  The USB 2.0 PHY driver is designed to support the XHCI controller. The SoCs
> +  support multiple XHCI controllers. One PHY device node maps to one XHCI
> +  controller.
> +
> +  RTD1295/RTD1619 SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY and map one USB 3.0 PHY on some
> +  controllers.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +
> +  RTD1395 SoCs USB
> +  The USB architecture includes two XHCI controllers.
> +  The controller#0 has one USB 2.0 PHY. The controller#1 includes two USB 2.0
> +  PHY.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +                               |- phy#1
> +
> +  RTD1319/RTD1619b SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY and map one USB 3.0 PHY on controllers#2.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +
> +  RTD1319d SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each xhci maps to one USB 2.0 PHY and map one USB 3.0 PHY on controllers#0.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +
> +  RTD1312c/RTD1315e SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - realtek,rtd1295-usb2phy
> +          - realtek,rtd1395-usb2phy
> +          - realtek,rtd1619-usb2phy
> +          - realtek,rtd1319-usb2phy
> +          - realtek,rtd1619b-usb2phy
> +          - realtek,rtd1312c-usb2phy
> +          - realtek,rtd1319d-usb2phy
> +          - realtek,rtd1315e-usb2phy

Keep entries ordered alphabetically.

> +      - const: realtek,usb2phy
> +
> +  reg:
> +    items:
> +      - description: PHY data registers
> +      - description: PHY control registers
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  "#phy-cells":
> +    const: 0
> +
> +  realtek,usb-ctrl:
> +    description: The phandle of syscon used to control USB PHY power domain.
> +    $ref: /schemas/types.yaml#/definitions/phandle

No, we have power-domains for this.

> +
> +patternProperties:
> +  "^phy@[0-3]+$":
> +    type: object
> +    description:
> +      Each sub-node is a PHY device for one XHCI controller.

I don't think it is true. You claim above that you have 0 as phy-cells,
means you have one phy. Here you say you can have up to 4 phys.

> +      For most Relatek SoCs, one XHCI controller only support one the USB 2.0
> +      phy. For RTD1395 SoC, the one XHCI controller has two USB 2.0 PHYs.
> +    properties:
> +      realtek,page0-param:
> +        description: PHY parameter at page 0. The data are the pair of the
> +          offset and value.

This needs to be specific. What the heck is "PHY parameter"?

> +        $ref: /schemas/types.yaml#/definitions/uint32-array

Array? Then maxItems.

> +
> +      realtek,page1-param:
> +        description: PHY parameter at page 1. The data are the pair of the
> +          offset and value.
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +
> +      realtek,page2-param:
> +        description: PHY parameter at page 2. The data are the pair of the
> +          offset and value. If the PHY support the page 2 parameter.
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +
> +      realtek,support-page2-param:
> +        description: Set this flag if PHY support page 2 parameter.

Why this cannot be deducted from compatible?

> +        type: boolean
> +
> +      realtek,do-toggle:
> +        description: Set this flag to enable PHY parameter toggle when port
> +          status change.

Do not instruct OS what to do. Explain why this is a hardware
characteristic.

> +        type: boolean
> +
> +      realtek,do-toggle-driving:
> +        description: Set this flag to enable PHY parameter toggle for adjust
> +          the driving when port status change.

Do not instruct OS what to do. Explain why this is a hardware
characteristic.


> +        type: boolean
> +
> +      realtek,check-efuse:
> +        description: Enable to update PHY parameter from reading otp table.

Do not instruct OS what to do. Explain why this is a hardware
characteristic.

> +        type: boolean
> +
> +      realtek,use-default-parameter:
> +        description: Don't set parameter and use default value in hardware.

NAK, you are just making things up.

> +        type: boolean
> +
> +      realtek,is-double-sensitivity-mode:
> +        description: Set this flag to enable double sensitivity mode.

All your descriptions copy the name of property. You basically say
nothing more. I already mentioned this before. Don't ignore the
feedback, but address it.

> +        type: boolean
> +
> +      realtek,ldo-force-enable:
> +        description: Set this flag to force enable ldo mode.

Drop everywhere "Set this flag to", because it is redundant. Now compare
what is left with property name.

Property name: realtek,ldo-force-enable
Your description: "force enable ldo mode"

How is this helpful to anybody?


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 5/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 3.0 PHY

[Krzysztof Kozlowski]

On 07/06/2023 08:24, Stanley Chang wrote:
> Add the documentation explain the property about Realtek USB PHY driver.
> 
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> controller. Added the driver to drive the  USB 3.0 PHY transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> v2 to v3 change:
>     1. Broken down into two patches, one for each of USB 2 & 3.
>     2. Add more description about Realtek RTD SoCs architecture.
>     3. Removed parameter v1 support for simplification.
>     4. Revised the compatible name for fallback compatible.
>     5. Remove some properties that can be set in the driver.
> v1 to v2 change:
>     Add phy-cells for generic phy driver
> ---
>  .../bindings/phy/realtek,usb3phy.yaml         | 156 ++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml b/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
> new file mode 100644
> index 000000000000..b45c398bba5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,usb3phy.yaml
> @@ -0,0 +1,156 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Realtek Semiconductor Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,usb3phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC SoCs USB 3.0 PHY
> +
> +maintainers:
> +  - Stanley Chang <stanley_chang@realtek.com>
> +
> +description:
> +  Realtek USB 3.0 PHY support the digital home center (DHC) RTD series SoCs.
> +  The USB 3.0 PHY driver is designed to support the XHCI controller. The SoCs
> +  support multiple XHCI controllers. One PHY device node maps to one XHCI
> +  controller.
> +
> +  RTD1295/RTD1619 SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY and map one USB 3.0 PHY on some
> +  controllers.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +
> +  RTD1395 SoCs USB
> +  The USB architecture includes two XHCI controllers.
> +  The controller#0 has one USB 2.0 PHY. The controller#1 includes two USB 2.0
> +  PHY.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +                               |- phy#1

Skip unrelated devices.

> +
> +  RTD1319/RTD1619b SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY and map one USB 3.0 PHY on controllers#2.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +
> +  RTD1319d SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each xhci maps to one USB 2.0 PHY and map one USB 3.0 PHY on controllers#0.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +                    |- usb3phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +
> +  RTD1312c/RTD1315e SoCs USB
> +  The USB architecture includes three XHCI controllers.
> +  Each XHCI maps to one USB 2.0 PHY.
> +  XHCI controller#0 -- usb2phy -- phy#0
> +  XHCI controller#1 -- usb2phy -- phy#0
> +  XHCI controller#2 -- usb2phy -- phy#0
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - realtek,rtd1295-usb3phy
> +          - realtek,rtd1619-usb3phy
> +          - realtek,rtd1319-usb3phy
> +          - realtek,rtd1619b-usb3phy
> +          - realtek,rtd1319d-usb3phy

Same comments as for previous patch.

> +      - const: realtek,usb3phy
> +
> +  reg:
> +    description: PHY data registers
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  "#phy-cells":
> +    const: 0

1 phy or 4? Decide.

> +
> +patternProperties:
> +  "^phy@[0-3]+$":
> +    description: Each sub-node is a PHY device for one XHCI controller.
> +    type: object
> +    properties:
> +      realtek,param:
> +        description: The data of PHY parameter are the pair of the
> +          offset and value.
> +        $ref: /schemas/types.yaml#/definitions/uint8-array

Your choice of types is surprising. If this is array, than maxItems (and
please don't tell me it is maxItems: 1). Anyway, why 8 bits long?

> +
> +      realtek,do-toggle:
> +        description: Set this flag to enable the PHY parameter toggle
> +          when port status change.
> +        type: boolean
> +
> +      realtek,do-toggle-once:
> +        description: Set this flag to do PHY parameter toggle only on
> +          PHY init.
> +        type: boolean
> +
> +      realtek,check-efuse:
> +        description: Enable to update PHY parameter from reading otp table.
> +        type: boolean
> +
> +      realtek,use-default-parameter:
> +        description: Don't set parameter and use default value in hardware.
> +        type: boolean
> +
> +      realtek,check-rx-front-end-offset:
> +        description: Enable to check rx front end offset.
> +        type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    usb_port2_usb3phy: usb-phy@13e10 {
> +        compatible = "realtek,rtd1319d-usb3phy", "realtek,usb3phy";
> +        reg = <0x13e10 0x4>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        #phy-cells = <0>;
> +
> +        phy@0 {
> +            reg = <0>;
> +            realtek,param =
> +                    <0x01 0xac8c>,
> +                    <0x06 0x0017>,

First, this is matrix, not uint8 array. Second, 0xac8c is past 16 bits
long, not 8. Third, you put some magic register programming to DT.
Please don't. Drop all this from DT.


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

[kernel test robot]

Hi Stanley,

kernel test robot noticed the following build errors:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus robh/for-next linus/master v6.4-rc5 next-20230607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanley-Chang/phy-realtek-usb-Add-driver-for-the-Realtek-SoC-USB-2-0-PHY/20230607-142704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230607062500.24669-2-stanley_chang%40realtek.com
patch subject: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY
config: arm64-randconfig-r014-20230607 (https://download.01.org/0day-ci/archive/20230608/202306080128.Gh3c2H1O-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add usb https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
        git fetch usb usb-testing
        git checkout usb/usb-testing
        b4 shazam https://lore.kernel.org/r/20230607062500.24669-2-stanley_chang@realtek.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306080128.Gh3c2H1O-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: "devm_usb_get_phy_by_phandle" [drivers/power/supply/wm831x_power.ko] undefined!
>> ERROR: modpost: "devm_usb_get_phy" [drivers/power/supply/da9150-charger.ko] undefined!

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for USB_PHY
   Depends on [n]: USB_SUPPORT [=n]
   Selected by [y]:
   - PHY_RTK_RTD_USB2PHY [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY

[kernel test robot]

Hi Stanley,

kernel test robot noticed the following build errors:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus robh/for-next linus/master v6.4-rc5 next-20230607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanley-Chang/phy-realtek-usb-Add-driver-for-the-Realtek-SoC-USB-2-0-PHY/20230607-142704
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230607062500.24669-3-stanley_chang%40realtek.com
patch subject: [PATCH v3 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20230608/202306080940.5Lcjwfck-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add usb https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
        git fetch usb usb-testing
        git checkout usb/usb-testing
        b4 shazam https://lore.kernel.org/r/20230607062500.24669-3-stanley_chang@realtek.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=s390 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306080940.5Lcjwfck-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: "usb_remove_phy" [drivers/phy/realtek/phy-rtk-usb2.ko] undefined!
ERROR: modpost: "usb_debug_root" [drivers/phy/realtek/phy-rtk-usb2.ko] undefined!
ERROR: modpost: "of_iomap" [drivers/phy/realtek/phy-rtk-usb2.ko] undefined!
ERROR: modpost: "usb_add_phy_dev" [drivers/phy/realtek/phy-rtk-usb2.ko] undefined!
>> ERROR: modpost: "usb_remove_phy" [drivers/phy/realtek/phy-rtk-usb3.ko] undefined!
>> ERROR: modpost: "usb_debug_root" [drivers/phy/realtek/phy-rtk-usb3.ko] undefined!
>> ERROR: modpost: "of_iomap" [drivers/phy/realtek/phy-rtk-usb3.ko] undefined!
>> ERROR: modpost: "usb_add_phy_dev" [drivers/phy/realtek/phy-rtk-usb3.ko] undefined!
ERROR: modpost: "devm_ioremap_resource" [drivers/dma/qcom/hdma.ko] undefined!
ERROR: modpost: "devm_platform_ioremap_resource" [drivers/dma/fsl-edma.ko] undefined!
WARNING: modpost: suppressed 26 unresolved symbol warnings because there were too many)

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for USB_PHY
   Depends on [n]: USB_SUPPORT [=n]
   Selected by [m]:
   - PHY_RTK_RTD_USB2PHY [=m]
   - PHY_RTK_RTD_USB3PHY [=m]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 08/06/2023 04:18, Stanley Chang[昌育德] wrote:
>> ERROR: modpost: "devm_usb_get_phy_by_phandle"
>> [drivers/power/supply/wm831x_power.ko] undefined!
>>>> ERROR: modpost: "devm_usb_get_phy"
>> [drivers/power/supply/da9150-charger.ko] undefined!
>>
>> Kconfig warnings: (for reference only)
>>    WARNING: unmet direct dependencies detected for USB_PHY
>>    Depends on [n]: USB_SUPPORT [=n]
>>    Selected by [y]:
>>    - PHY_RTK_RTD_USB2PHY [=y]
>>
> 
> I will add USB_SUUPRT dependency to Kconfig.

Why? Do you see other phy drivers needing it? Few have it but many
don't, so you should really investigate the root cause, not just add
some dependencies.

Build test your patches locally before sending and investigate the issues.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 08/06/2023 08:59, Stanley Chang[昌育德] wrote:
> Hi Krzysztof,
> 
> 
>> I commented on your second patch, but everything is applicable here as well.
>> You have many useless debug messages. Many incorrect or useless
>> "if() return" which point to broken driver design (e.g. concurrent access to half
>> initialized structures where you substitute lack of synchronization with
>> incorrect "if() return"). Undocumented user interface is one more big trouble.
>>
>> I doubt you run checkpatch on this (be sure to run it with --strict and fix almost
>> everything).
>>
>>
> 1. I use checkpatch but without --strict. I will add it add and check again.
> 2. Do the debugfs interfaces need to provide a document?
> I don't find any reference about this.

debugfs interface is for debug but in your case you don't use it that way.



Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 08/06/2023 08:59, Stanley Chang[昌育德] wrote:
> Hi Krzysztof,
> 
> 
>> I commented on your second patch, but everything is applicable here as well.
>> You have many useless debug messages. Many incorrect or useless
>> "if() return" which point to broken driver design (e.g. concurrent access to half
>> initialized structures where you substitute lack of synchronization with
>> incorrect "if() return"). Undocumented user interface is one more big trouble.
>>
>> I doubt you run checkpatch on this (be sure to run it with --strict and fix almost
>> everything).
>>
>>
> 1. I use checkpatch but without --strict. I will add it add and check again.

Around 300 issues pointed out...

> 2. Do the debugfs interfaces need to provide a document?
> I don't find any reference about this.

Not sure if we have it in docs, but it is discussed every now and then...

https://lore.kernel.org/all/CAMuHMdUaugQ5+Zhmg=oe=X2wvhazMiT=K-su0EJYKzD4Hdyn3Q@mail.gmail.com/
https://lore.kernel.org/all/2023060209-scouts-affection-f54d@gregkh/



Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY

[Krzysztof Kozlowski]

On 08/06/2023 08:59, Stanley Chang[昌育德] wrote:
> Hi Krzysztof,
> 
>>> +static void do_rtk_usb3_phy_toggle(struct rtk_usb_phy *rtk_phy, int i,
>>> +         bool isConnect)
>>> +{
>>> +     struct reg_addr *regAddr;
>>> +     struct phy_data *phy_data;
>>> +     struct phy_parameter *phy_parameter;
>>> +     size_t index;
>>> +
>>> +     regAddr = &((struct reg_addr *)rtk_phy->reg_addr)[i];
>>> +     phy_data = &((struct phy_data *)rtk_phy->phy_data)[i];
>>> +
>>> +     if (!phy_data) {
>>> +             dev_err(rtk_phy->dev, "%s phy_data is NULL!\n",
>>> + __func__);
>>
>> ???
> Sorry, this check is redundant.
> 
>>
>>> +             return;
>>> +     }
>>> +
>>> +     if (!phy_data->do_toggle)
>>> +             return;
>>> +
>>> +     phy_parameter = phy_data->parameter;
>>> +
>>> +     index = PHY_ADDR_MAP_ARRAY_INDEX(PHY_ADDR_0x09);
>>> +
>>> +     if (index < phy_data->size) {
>>> +             u8 addr = (phy_parameter + index)->addr;
>>> +             u16 data = (phy_parameter + index)->data;
>>> +
>>> +             if (addr == 0xFF) {
>>> +                     addr = ARRAY_INDEX_MAP_PHY_ADDR(index);
>>> +                     data = rtk_usb_phy_read(regAddr, addr);
>>> +                     (phy_parameter + index)->addr = addr;
>>> +                     (phy_parameter + index)->data = data;
>>> +             }
>>> +             mdelay(1);
>>> +             dev_info(rtk_phy->dev,
>>> +                         "%s ########## to toggle PHY addr 0x09
>> BIT(9)\n",
>>> +                         __func__);
>>> +             rtk_usb_phy_write(regAddr, addr, data&(~BIT(9)));
>>> +             mdelay(1);
>>> +             rtk_usb_phy_write(regAddr, addr, data);
>>> +     }
>>> +     dev_info(rtk_phy->dev, "%s ########## PHY addr 0x1f = 0x%04x\n",
>>> +                 __func__, rtk_usb_phy_read(regAddr,
>> PHY_ADDR_0x1F));
>>
>> Please drop all simple debug success messages. Linux has already
>> infrastructure for this.
>>
> Okay. I will change the print dev_info to dev_dbg about debug message.

No, drop them. This piece of code had already 2 printks for register
contents! Your driver is overloaded with printks and they are mostly
useless for the user.

> 
>> ...
>>
>>> +     return 0;
>>> +}
>>> +
>>> +static int rtk_usb_phy_init(struct phy *phy) {
>>> +     struct rtk_usb_phy *rtk_phy = phy_get_drvdata(phy);
>>> +     int ret = 0;
>>> +     int i;
>>> +     unsigned long phy_init_time = jiffies;
>>> +
>>> +     if (!rtk_phy) {
>>> +             pr_err("%s rtk_phy is NULL!\n", __func__);
>>
>> What? How is this possible?
> It should be not necessary. I will remove it.
> 
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     dev_dbg(rtk_phy->dev, "Init RTK USB 3.0 PHY\n");
>>> +     for (i = 0; i < rtk_phy->phyN; i++)
>>> +             ret = do_rtk_usb_phy_init(rtk_phy, i);
>>> +
>>> +     dev_info(rtk_phy->dev, "Initialized RTK USB 3.0 PHY (take %dms)\n",
>>> +                 jiffies_to_msecs(jiffies - phy_init_time));
>>
>> Please drop all simple debug success messages. Linux has already
>> infrastructure for this.
> 
> Ok, Thanks.
> 
>>> +     return ret;
>>> +}
>>> +
>>> +static int rtk_usb_phy_exit(struct phy *phy) {
>>> +     struct rtk_usb_phy *rtk_phy = phy_get_drvdata(phy);
>>> +
>>> +     if (!rtk_phy) {
>>> +             pr_err("%s rtk_phy is NULL!\n", __func__);
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     dev_dbg(rtk_phy->dev, "Exit RTK USB 3.0 PHY\n");
>>
>> Please drop all simple debug success messages. Linux has already
>> infrastructure for this.
> 
> Can I keep log for dev_dbg?

Of course not. This was dev_dbg and I commented on this. This is not a
good debug, we do not print anything on function entrance and exit.
ftrace() is for this.

> 
>>> +static void rtk_usb_phy_toggle(struct usb_phy *usb3_phy, bool
>>> +isConnect, int port) {
>>> +     int index = port;
>>> +     struct rtk_usb_phy *rtk_phy = NULL;
>>> +
>>> +     if (usb3_phy != NULL && usb3_phy->dev != NULL)
>>> +             rtk_phy = dev_get_drvdata(usb3_phy->dev);
>>> +
>>> +     if (rtk_phy == NULL) {
>>> +             pr_err("%s ERROR! NO this device\n", __func__);
>>
>> Your error messages are not helping. No need to shout, no need to handle
>> some non-existing cases. If this is real case, you have broken driver. I actually
>> suspect that.
>>
>> How can you interface with a driver where there is no device?
> 
> OK, I know this is not good programming practice, I will improve this question.
> 
>>> +             return;
>>> +     }
>>> +
>>> +     if (index > rtk_phy->phyN) {
>>> +             pr_err("%s %d ERROR! port=%d > phyN=%d\n",
>>> +                         __func__, __LINE__, index, rtk_phy->phyN);
>>> +             return;
>>> +     }
>>> +
>>> +     do_rtk_usb3_phy_toggle(rtk_phy, index, isConnect); }
>>> +
>>> +static int rtk_usb_phy_notify_port_status(struct usb_phy *x, int port,
>>> +         u16 portstatus, u16 portchange) {
>>> +     bool isConnect = false;
>>
>> This is not C++. Don't use camelcase. See Coding style document.
> 
> I will revised for this style.
> 
>>> +
>>> +     pr_debug("%s port=%d portstatus=0x%x portchange=0x%x\n",
>>> +                 __func__, port, (int)portstatus, (int)portchange);
>>> +     if (portstatus & USB_PORT_STAT_CONNECTION)
>>> +             isConnect = true;
>>> +
>>
>> ...
>>
>>> +
>>> +static int rtk_usb3_set_parameter_show(struct seq_file *s, void
>>> +*unused) {
>>> +     struct rtk_usb_phy *rtk_phy = s->private;
>>> +     const struct file *file = s->file;
>>> +     const char *file_name = file_dentry(file)->d_iname;
>>> +     struct dentry *p_dentry = file_dentry(file)->d_parent;
>>> +     const char *phy_dir_name = p_dentry->d_iname;
>>> +     int ret, index;
>>> +     struct phy_data *phy_data = NULL;
>>> +
>>> +     for (index = 0; index < rtk_phy->phyN; index++) {
>>> +             size_t sz = 30;
>>> +             char name[30] = {0};
>>> +
>>> +             snprintf(name, sz, "phy%d", index);
>>> +             if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
>>> +                     phy_data = &((struct phy_data
>> *)rtk_phy->phy_data)[index];
>>> +                     break;
>>> +             }
>>> +     }
>>> +     if (!phy_data) {
>>> +             dev_err(rtk_phy->dev,
>>> +                                 "%s: No phy_data for %s/%s\n",
>>> +                                 __func__, phy_dir_name,
>> file_name);
>>
>> Mess wrapping/indentation. Actually everywhere in the file...
> 
> I will improve this.
> 
>>> +static int rtk_usb3_set_parameter_open(struct inode *inode, struct
>>> +file *file) {
>>> +     return single_open(file, rtk_usb3_set_parameter_show,
>>> +inode->i_private); }
>>> +
>>> +static ssize_t rtk_usb3_set_parameter_write(struct file *file,
>>> +             const char __user *ubuf, size_t count, loff_t *ppos) {
>>> +     const char *file_name = file_dentry(file)->d_iname;
>>> +     struct dentry *p_dentry = file_dentry(file)->d_parent;
>>> +     const char *phy_dir_name = p_dentry->d_iname;
>>> +     struct seq_file         *s = file->private_data;
>>> +     struct rtk_usb_phy              *rtk_phy = s->private;
>>> +     struct reg_addr *regAddr = NULL;
>>> +     struct phy_data *phy_data = NULL;
>>> +     int ret = 0;
>>> +     char buffer[40] = {0};
>>> +     int index;
>>> +
>>> +     if (copy_from_user(&buffer, ubuf,
>>> +                 min_t(size_t, sizeof(buffer) - 1, count)))
>>> +             return -EFAULT;
>>> +
>>> +     for (index = 0; index < rtk_phy->phyN; index++) {
>>> +             size_t sz = 30;
>>> +             char name[30] = {0};
>>> +
>>> +             snprintf(name, sz, "phy%d", index);
>>> +             if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
>>> +                     regAddr = &((struct reg_addr
>> *)rtk_phy->reg_addr)[index];
>>> +                     phy_data = &((struct phy_data
>> *)rtk_phy->phy_data)[index];
>>> +                     break;
>>
>>
>> Where is the ABI documentation for user interface?
> 
> Do debugfs nodes need ABI documentation?
> Is there a reference?
>>
>>> +
>>> +static inline void create_debug_files(struct rtk_usb_phy *rtk_phy) {
>>> +     struct dentry *phy_debug_root = NULL;
>>> +     struct dentry *set_parameter_dir = NULL;
>>> +
>>> +     phy_debug_root = create_phy_debug_root();
>>> +
>>> +     if (!phy_debug_root) {
>>> +             dev_err(rtk_phy->dev, "%s Error phy_debug_root is NULL",
>>> +                         __func__);
>>> +             return;
>>> +     }
>>> +     rtk_phy->debug_dir = debugfs_create_dir(dev_name(rtk_phy->dev),
>>> +                 phy_debug_root);
>>> +     if (!rtk_phy->debug_dir) {
>>> +             dev_err(rtk_phy->dev, "%s Error debug_dir is NULL",
>>> + __func__);
>>
>> Are you sure you run checkpatch on this? Error messages on debugfs are
>> almost always incorrect.
> 
> Yes, I have run checkpatch for patches. 
> Why the message is incorrect?

Because debugfs failures should not cause any error prints. It's debug,
not important.

Do you see anywhere error messages?

Entire debugfs handling code should be silent and even skip all error
checking, as most API is ready for handling previous errors, I think.

> 
>>> +static int get_phy_parameter(struct rtk_usb_phy *rtk_phy,
>>> +         struct device_node *sub_node) {
>>> +     struct device *dev = rtk_phy->dev;
>>> +     struct reg_addr *addr;
>>> +     struct phy_data *phy_data;
>>> +     int ret = 0;
>>> +     int index;
>>> +
>>> +     if (of_property_read_u32(sub_node, "reg", &index)) {
>>> +             dev_err(dev, "sub_node without reg\n");
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     dev_dbg(dev, "sub_node index=%d\n", index);
>>
>> Please drop all simple debug success messages. Linux has already
>> infrastructure for this.
> 
> Can I keep log for dev_dbg?

No, this was dev_dbg and I commented on this to remove it. Keep only
useful debug messages, not hundreds of them in every place.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 4/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 08/06/2023 09:24, Stanley Chang[昌育德] wrote:
>>> +  realtek,usb-ctrl:
>>> +    description: The phandle of syscon used to control USB PHY power
>> domain.
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>
>> No, we have power-domains for this.
> 
> Maybe I use the word "control power domain" is not well, I just want to control the ldo of usb phy.
> Revised:
> The phandle of syscon used to control the ldo of USB PHY.

Isn't this still a power domain?

> 
>>> +
>>> +patternProperties:
>>> +  "^phy@[0-3]+$":
>>> +    type: object
>>> +    description:
>>> +      Each sub-node is a PHY device for one XHCI controller.
>>
>> I don't think it is true. You claim above that you have 0 as phy-cells, means you
>> have one phy. Here you say you can have up to 4 phys.
> 
> I mean the driver can support up to 4 phys.

What driver can or cannot do, does not matter. This is about hardware.

> For RTD1295 has only one phy.
> For RTD1395 has two phys.

Two phys? So how do you reference them when cells=0?

> 
>>> +      For most Relatek SoCs, one XHCI controller only support one the USB
>> 2.0
>>> +      phy. For RTD1395 SoC, the one XHCI controller has two USB 2.0
>> PHYs.
>>> +    properties:
>>> +      realtek,page0-param:
>>> +        description: PHY parameter at page 0. The data are the pair of
>> the
>>> +          offset and value.
>>
>> This needs to be specific. What the heck is "PHY parameter"?
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>
>> Array? Then maxItems.
> I have found other document.
> It should be a uint32-matrix.
> I will add the maxItems.

Entire property should be dropped.

> 
>>> +
>>> +      realtek,page1-param:
>>> +        description: PHY parameter at page 1. The data are the pair of
>> the
>>> +          offset and value.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +
>>> +      realtek,page2-param:
>>> +        description: PHY parameter at page 2. The data are the pair of
>> the
>>> +          offset and value. If the PHY support the page 2 parameter.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +
>>> +      realtek,support-page2-param:
>>> +        description: Set this flag if PHY support page 2 parameter.
>>
>> Why this cannot be deducted from compatible?
> It can identify by compatible.

So drop it.

> 
>>
>>> +        type: boolean
>>> +
>>> +      realtek,do-toggle:
>>> +        description: Set this flag to enable PHY parameter toggle when
>> port
>>> +          status change.
>>
>> Do not instruct OS what to do. Explain why this is a hardware characteristic.
> 
> In my original intention, we hope that this property can be used to control the phy driver do parameter toggle.
> Is it a hardware characteristic? I don't think it's exactly a hardware feature.
> Maybe it can be specified by the compatible.

Drop it.

> 
>>> +        type: boolean
>>> +
>>> +      realtek,do-toggle-driving:
>>> +        description: Set this flag to enable PHY parameter toggle for
>> adjust
>>> +          the driving when port status change.
>>
>> Do not instruct OS what to do. Explain why this is a hardware characteristic.
>>
>>
>>> +        type: boolean
>>> +
>>> +      realtek,check-efuse:
>>> +        description: Enable to update PHY parameter from reading otp
>> table.
>>
>> Do not instruct OS what to do. Explain why this is a hardware characteristic.
> 
> Same above.

So drop all of these.


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 5/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 3.0 PHY

[Krzysztof Kozlowski]

On 08/06/2023 09:32, Stanley Chang[昌育德] wrote:
>>> +examples:
>>> +  - |
>>> +    usb_port2_usb3phy: usb-phy@13e10 {
>>> +        compatible = "realtek,rtd1319d-usb3phy", "realtek,usb3phy";
>>> +        reg = <0x13e10 0x4>;
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +        #phy-cells = <0>;
>>> +
>>> +        phy@0 {
>>> +            reg = <0>;
>>> +            realtek,param =
>>> +                    <0x01 0xac8c>,
>>> +                    <0x06 0x0017>,
>>
>> First, this is matrix, not uint8 array. Second, 0xac8c is past 16 bits long, not 8.
>> Third, you put some magic register programming to DT.
>> Please don't. Drop all this from DT.
> 
> realtek,param is an uint32-matrx.
> I will revised the type.

Drop the property. It is not explained and not justified to be in DT.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 4/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 08/06/2023 09:47, Stanley Chang[昌育德] wrote:
> Hi Krzysztof,
> 
> 
>>> +      For most Relatek SoCs, one XHCI controller only support one the USB
>> 2.0
>>> +      phy. For RTD1395 SoC, the one XHCI controller has two USB 2.0
>> PHYs.
>>> +    properties:
>>> +      realtek,page0-param:
>>> +        description: PHY parameter at page 0. The data are the pair of
>> the
>>> +          offset and value.
>>
>> This needs to be specific. What the heck is "PHY parameter"?
>>
> It contains more parameters
> page0 has 16 parameters
> page1 has 8 parameters
> page2 has 8 parameters
> It's tedious if we list them all.

Sure, if you prefer not to list them, then they should be removed from DT.

> And we only set the part that differs from the default.
> It's hard to explain which parameters were changed because each platform is different.

If this is phy tuning per board, you need to explain and justify them.
If this is per platform, then drop it - not even needed, because you
have compatible for this.


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 4/5] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0 PHY

[Krzysztof Kozlowski]

On 08/06/2023 10:21, Stanley Chang[昌育德] wrote:
> 
>>> Maybe I use the word "control power domain" is not well, I just want to
>> control the ldo of usb phy.
>>> Revised:
>>> The phandle of syscon used to control the ldo of USB PHY.
>>
>> Isn't this still a power domain?
> 
> I only control a register, it is not needed a driver of power domain.

Aren't many power domains just a registers? What about other drivers?
Don't you want in other driver control LDO of something else? And in
other something else?

> 
> 
>>>
>>>>> +
>>>>> +patternProperties:
>>>>> +  "^phy@[0-3]+$":
>>>>> +    type: object
>>>>> +    description:
>>>>> +      Each sub-node is a PHY device for one XHCI controller.
>>>>
>>>> I don't think it is true. You claim above that you have 0 as
>>>> phy-cells, means you have one phy. Here you say you can have up to 4 phys.
>>>
>>> I mean the driver can support up to 4 phys.
>>
>> What driver can or cannot do, does not matter. This is about hardware.
>>
>>> For RTD1295 has only one phy.
>>> For RTD1395 has two phys.
>>
>> Two phys? So how do you reference them when cells=0?
> 
> 
> About RTD1395 SoCs USB
>   XHCI controller#1 -- usb2phy -- phy#0
>                           |- phy#1
> One xhci controller map to one phy driver.
> And one phy driver have two phys (phy@0 and phy@1).
> 
> Maybe the "phy" name is confusing.
> This "phy" not mean a phy driver.

We do not talk about drivers, but DTS and hardware.

> Would "port" be more appropriate? 
> 
> For example,
> Using phy@0 and phy@1:
>     usb_port1_usb2phy: usb-phy@13c14 {
>         compatible = "realtek,rtd1395-usb2phy", "realtek,usb2phy";
>         reg = <0x132c4 0x4>, <0x31280 0x8>;
>         #address-cells = <1>;
>         #size-cells = <0>;
>         #phy-cells = <0>;
>         realtek,usb-ctrl = <&usb_ctrl>;
> 
>         phy@0 {
>             reg = <0>;

So such child is a NAK... you have nothing here. But it's unrelated topic.

>         };
>         phy@1 {
>             reg = <1>;
>         };
>     };
> 
> Change: port@0 and port@1
>     usb_port1_usb2phy: usb-phy@13c14 {
>         compatible = "realtek,rtd1395-usb2phy", "realtek,usb2phy";
>         reg = <0x132c4 0x4>, <0x31280 0x8>;
>         #address-cells = <1>;
>         #size-cells = <0>;
>         #phy-cells = <0>;
>         realtek,usb-ctrl = <&usb_ctrl>;
> 
>         prot@0 {
>             reg = <0>;
>         };
>         port@1 {
>             reg = <1>;
>         };
>     };

This is not the answer. This is the provider. How do you reference it
from the consumer.

Upstream your entire DTS. It's frustrating to try to understand your DTS
from pieces of information you are sharing. Also very time consuming and
you are not the only one sending patches for review...

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy