| Message ID | 20230607062500.24669-1-stanley_chang@realtek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/5] usb: phy: add usb phy notify port status API | expand |
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. > ... > > > + 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? > > +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? > > +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? > ... > > > + > > +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? You are right. The of_node is always exist. I will remove it. > > > + 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. Okay. I will remove debug message. Thanks, Stanley
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
Hi Krzysztof, > > 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. Okay. > > + - 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. 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. > > + > > +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. For RTD1295 has only one phy. For RTD1395 has two 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. I have found other document. It should be a uint32-matrix. I will add the 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? It can identify by 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. 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. > > + 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. > > + type: boolean > > + > > + realtek,use-default-parameter: > > + description: Don't set parameter and use default value in > hardware. > > NAK, you are just making things up. This is a software flow control. I will remove it. > > > + 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. I will improve this. > > + 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? This is a software flow control. I will remove it. Thanks, Stanley .
> >> 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. I will drop them to simplify the code. > >> 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. Well, for debugging purposes, I'm going to have to dig into ftrace. This is a great tip. > >> > >> 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. Thanks, I understand now. Thanks, Stanley
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
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
> >> > >> 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. > Okay, I try to specify by the compatible. Thanks, Stanley
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
> > 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? I will use power domain to instead this. > > 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. Here is for simple, so some items ignore. > > }; > > 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... Sorry to take up a lot of your time. Apparently I don't know enough about dts. I will reference more device tree document to understand the relating between DTS and hardware. Thanks, Stanley
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 97a0f8faea6e..b4fbbeae1927 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub, int port1, int type, ret = 0; } mutex_unlock(&hub->status_mutex); + + if (!ret) { + struct usb_device *hdev = hub->hdev; + + if (hdev && !hdev->parent) { + struct usb_hcd *hcd = bus_to_hcd(hdev->bus); + + if (hcd->usb_phy) + usb_phy_notify_port_status(hcd->usb_phy, + port1 - 1, *status, *change); + } + } + return ret; } diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h index e4de6bc1f69b..53bf3540098f 100644 --- a/include/linux/usb/phy.h +++ b/include/linux/usb/phy.h @@ -144,6 +144,10 @@ struct usb_phy { */ int (*set_wakeup)(struct usb_phy *x, bool enabled); + /* notify phy port status change */ + int (*notify_port_status)(struct usb_phy *x, + int port, u16 portstatus, u16 portchange); + /* notify phy connect status change */ int (*notify_connect)(struct usb_phy *x, enum usb_device_speed speed); @@ -316,6 +320,16 @@ usb_phy_set_wakeup(struct usb_phy *x, bool enabled) return 0; } +static inline int +usb_phy_notify_port_status(struct usb_phy *x, int port, u16 portstatus, + u16 portchange) +{ + if (x && x->notify_port_status) + return x->notify_port_status(x, port, portstatus, portchange); + else + return 0; +} + static inline int usb_phy_notify_connect(struct usb_phy *x, enum usb_device_speed speed) {
In Realtek SoC, the parameter of usb phy is designed to can dynamic tuning base on port status. Therefore, add a notify callback of phy driver when usb port status change. The Realtek phy driver is designed to dynamically adjust disconnection level and calibrate phy parameters. When the device connected bit changes and when the disconnected bit changes, do port status change notification: Check if portstatus is USB_PORT_STAT_CONNECTION and portchange is USB_PORT_STAT_C_CONNECTION. 1. The device is connected, the driver lowers the disconnection level and calibrates the phy parameters. 2. The device disconnects, the driver increases the disconnect level and calibrates the phy parameters. When controller to notify connect that device is already ready. If we adjust the disconnection level in notify_connect, the disconnect may have been triggered at this stage. So we need to change that as early as possible. Therefore, we add an api to notify phy the port status changes. Signed-off-by: Stanley Chang <stanley_chang@realtek.com> --- v2 to v3 change: Add more comments about the reason for adding this api v1 to v2 change: No change --- drivers/usb/core/hub.c | 13 +++++++++++++ include/linux/usb/phy.h | 14 ++++++++++++++ 2 files changed, 27 insertions(+)