lkml.org 
[lkml]   [2023]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v4 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer driver
    Date
    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

    \
     
     \ /
      Last update: 2023-08-18 09:45    [W:8.834 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site