RE: [PATCH v4 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer driver

[Stanley Chang[昌育德]]

Hi Thinh,

> 
> On Tue, Aug 15, 2023, Stanley Chang wrote:
> > Realtek DHC RTD SoCs integrate dwc3 IP and has some customizations to
> > support different generations of SoCs.
> 
> Please provide a summary of what "customizations" are done here.
> 
I will add description:

The RTD1619b subclass SoC only supports USB 2.0 from dwc3.
The driver can set a maximum speed to support this.
Add role switching function, that can switch USB roles through other drivers,
or switch USB roles through user space through set /sys/class/usb_role/.

> > +struct dwc3_rtk {
> > +     struct device *dev;
> > +     void __iomem *regs;
> > +     size_t regs_size;
> > +     void __iomem *pm_base;
> > +
> > +     struct dwc3 *dwc;
> > +
> > +     int cur_dr_mode; /* current dr mode */
> 
> Why not use enum for dr_mode? And I don't think you need the comment.

I will remove comment.
I will modify to use enumeration and define usb_role uniformly instead of
dr_mode to avoid confusing dr_mode and usb_role.

> > +     struct usb_role_switch *role_switch; };
> > +
> > +static void switch_usb2_dr_mode(struct dwc3_rtk *rtk, int dr_mode) {
> > +     switch (dr_mode) {
> > +     case USB_DR_MODE_PERIPHERAL:
> > +             writel(USB2_PHY_SWITCH_DEVICE |
> > +                         (~USB2_PHY_SWITCH_MASK &
> > +                           readl(rtk->regs + WRAP_USB2_PHY_REG)),
> > +                         rtk->regs + WRAP_USB2_PHY_REG);
> 
> Please split the register read and write to separate operations here and
> everywhere else. ie:
>         val = readl(offset);
>         writel(val | mod, offset);
Okay.

> > +             break;
> > +     case USB_DR_MODE_HOST:
> > +             writel(USB2_PHY_SWITCH_HOST |
> > +                         (~USB2_PHY_SWITCH_MASK &
> > +                           readl(rtk->regs + WRAP_USB2_PHY_REG)),
> > +                         rtk->regs + WRAP_USB2_PHY_REG);
> > +             break;
> > +     default:
> > +             dev_dbg(rtk->dev, "%s: dr_mode=%d\n", __func__,
> dr_mode);
> > +             break;
> > +     }
> > +}
> > +
> > +static void switch_dwc3_dr_mode(struct dwc3_rtk *rtk, int dr_mode) {
> > +     if (!rtk->dwc->role_sw)
> > +             goto out;
> > +
> > +     switch (dr_mode) {
> > +     case USB_DR_MODE_PERIPHERAL:
> > +             usb_role_switch_set_role(rtk->dwc->role_sw,
> USB_ROLE_DEVICE);
> > +             break;
> > +     case USB_DR_MODE_HOST:
> > +             usb_role_switch_set_role(rtk->dwc->role_sw,
> USB_ROLE_HOST);
> > +             break;
> > +     default:
> > +             dev_dbg(rtk->dev, "%s dr_mode=%d\n", __func__,
> dr_mode);
> > +             break;
> > +     }
> > +
> > +out:
> > +     return;
> > +}
> > +
> > +static int dwc3_rtk_get_dr_mode(struct dwc3_rtk *rtk) {
> > +     enum usb_role role;
> > +
> > +     role = rtk->cur_dr_mode;
> > +
> > +     if (rtk->dwc && rtk->dwc->role_sw)
> > +             role = usb_role_switch_get_role(rtk->dwc->role_sw);
> > +     else
> > +             dev_dbg(rtk->dev, "%s not usb_role_switch role=%d\n",
> > + __func__, role);
> > +
> > +     return role;
> > +}
> > +
> > +static void dwc3_rtk_set_dr_mode(struct dwc3_rtk *rtk, int dr_mode) {
> > +     rtk->cur_dr_mode = dr_mode;
> > +
> > +     switch_dwc3_dr_mode(rtk, dr_mode);
> > +     mdelay(10);
> > +     switch_usb2_dr_mode(rtk, dr_mode); }
> > +
> > +#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH)
> > +static int dwc3_usb_role_switch_set(struct usb_role_switch *sw, enum
> > +usb_role role) {
> > +     struct dwc3_rtk *rtk = usb_role_switch_get_drvdata(sw);
> > +
> > +     switch (role) {
> > +     case USB_ROLE_HOST:
> > +             dwc3_rtk_set_dr_mode(rtk, USB_DR_MODE_HOST);
> > +             break;
> > +     case USB_ROLE_DEVICE:
> > +             dwc3_rtk_set_dr_mode(rtk, USB_DR_MODE_PERIPHERAL);
> > +             break;
> > +     default:
> > +             dwc3_rtk_set_dr_mode(rtk, 0);
> 
> Any other value should be invalid and should not invoke
> dwc3_rtk_set_dr_mode().

I will remove it.

> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch
> > +*sw) {
> > +     struct dwc3_rtk *rtk = usb_role_switch_get_drvdata(sw);
> > +     enum usb_role role = USB_ROLE_NONE;
> > +     int dr_mode;
> > +
> > +     dr_mode = dwc3_rtk_get_dr_mode(rtk);
> 
> dwc3_rtk_get_dr_mode() returns int converted from enum usb_role. Now
> you're mixing dr_mode with usb_role. Please use enum and avoid casting.

This is my fault. cur_dr_mode mixes dr_mode and usb_role, although they have the same value.
I will use cur_role and enum usb_role types uniformly.

> > +     switch (dr_mode) {
> > +     case USB_DR_MODE_HOST:
> > +             role = USB_ROLE_HOST;
> > +             break;
> > +     case USB_DR_MODE_PERIPHERAL:
> > +             role = USB_ROLE_DEVICE;
> > +             break;
> > +     default:
> > +             dev_dbg(rtk->dev, "%s dr_mode=%d", __func__, dr_mode);
> > +             break;
> > +     }
> > +     return role;
> > +}
> > +
> > +static int dwc3_rtk_setup_role_switch(struct dwc3_rtk *rtk) {
> > +     struct usb_role_switch_desc dwc3_role_switch = {NULL};
> > +
> > +     dwc3_role_switch.name = strchrnul(dev_name(rtk->dev), '.') + 1;
> 
> Why not just use dev_name(rtk->dev)?
> 
I want to remove the address.
For example,
Original:
98020000.dwc3_u2drd-role-switch
I want:
dwc3_u2drd-role-switch

> > +     dwc3_role_switch.driver_data = rtk;
> > +     dwc3_role_switch.allow_userspace_control = true;
> > +     dwc3_role_switch.fwnode = dev_fwnode(rtk->dev);
> > +     dwc3_role_switch.set = dwc3_usb_role_switch_set;
> > +     dwc3_role_switch.get = dwc3_usb_role_switch_get;
> > +     rtk->role_switch = usb_role_switch_register(rtk->dev,
> &dwc3_role_switch);
> > +     if (IS_ERR(rtk->role_switch))
> > +             return PTR_ERR(rtk->role_switch);
> > +
> > +     return 0;
> > +}
> > +
> > +static int dwc3_rtk_remove_role_switch(struct dwc3_rtk *rtk) {
> > +     if (rtk->role_switch)
> > +             usb_role_switch_unregister(rtk->role_switch);
> > +
> > +     rtk->role_switch = NULL;
> > +
> > +     return 0;
> > +}
> > +#else
> > +#define dwc3_rtk_setup_role_switch(x) 0 #define
> > +dwc3_rtk_remove_role_switch(x) 0 #endif
> > +


> > +static int dwc3_rtk_probe(struct platform_device *pdev) {
> > +     struct dwc3_rtk *rtk;
> > +     struct device *dev = &pdev->dev;
> > +     struct resource *res;
> > +     void __iomem *regs;
> > +     int ret = 0;
> > +     unsigned long probe_time = jiffies;
> > +
> > +     rtk = devm_kzalloc(dev, sizeof(*rtk), GFP_KERNEL);
> > +     if (!rtk) {
> > +             ret = -ENOMEM;
> > +             goto err1;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, rtk);
> > +
> > +     rtk->dev = dev;
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     if (!res) {
> > +             dev_err(dev, "missing memory resource\n");
> > +             ret = -ENODEV;
> > +             goto err1;
> > +     }
> > +
> > +     regs = devm_ioremap_resource(dev, res);
> > +     if (IS_ERR(regs)) {
> > +             ret = PTR_ERR(regs);
> > +             goto err1;
> > +     }
> > +
> > +     rtk->regs = regs;
> > +     rtk->regs_size = resource_size(res);
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +     if (res) {
> > +             rtk->pm_base = devm_ioremap_resource(dev, res);
> > +             if (IS_ERR(rtk->pm_base)) {
> > +                     ret = PTR_ERR(rtk->pm_base);
> > +                     goto err1;
> > +             }
> > +     }
> > +
> > +     ret = dwc3_rtk_probe_dwc3_core(rtk);
> > +     if (ret)
> > +             goto err1;
> > +
> > +     dev_dbg(dev, "%s ok! (take %d ms)\n", __func__,
> > +             jiffies_to_msecs(jiffies - probe_time));
> 
> This debug message doesn't look like it should be here unless it's early in the
> development cycle. Do we need this debug message?

I only want to print time of probe.
I will remove it.

> > +
> > +     return 0;
> > +
> > +err1:
> 
> Where's err2? If there are multiple gotos, provide more descriptive names
> instead of just numbers.

Okay I will revise this.
> 
> > +     return ret;
> > +}
> > +

Thanks,
Stanley