[U-Boot] 答复: [PATCH v2 1/4] USB: gadget: Add the cadence USB3 gadget driver
Sherry Sun
sherry.sun at nxp.com
Fri Aug 16 04:45:42 UTC 2019
Hi Marek
>
> On 8/14/19 2:16 PM, sherry sun wrote:
> > From: Sherry Sun <sherry.sun at nxp.com>
> >
> > This driver is ported from NXP i.MX U-Boot version imx_v2019.04 and
> > some changes have also been made to adapt to U-Boot.
> >
> > Add the Cadence USB3 IP(CDNS3) driver for the gadget (device mode).
> > The CDNS3 gadget driver support DM mode. CONFIG_DM_USB_GADGET
> should
> > be enabled when use this driver. And gadget_is_cdns3 checking is added
> > to provide bcdUSB value in device descriptor.
>
> The cadence core isn't xhci compatible ? Sigh ...
>
Actually, I'm not very understand what the xhci compatible means?
The cadence core can support usb peripheral and host. But for now,
we just done the gadget part in the core code. If it needed later, we
can add the host part to it.
> [...]
>
> > +++ b/doc/device-tree-bindings/usb/cdns-usb3.txt
> > @@ -0,0 +1,39 @@
> > +* Cadence USB3 Controller
> > +
> > +Required properties:
> > +- compatible: "Cadence,usb3";
>
> cdns , no ?
Yes, I have already changed all compatible to "cdns,usb3".
>
> [...]
>
> > +static int cdns3_generic_peripheral_clk_init(struct udevice *dev,
> > + struct cdns3_generic_peripheral
> > + *priv)
> > +{
> > + int ret;
> > +
> > + ret = clk_get_bulk(dev, &priv->clks);
> > + if (ret)
> > + return ret;
> > +
> > +#if CONFIG_IS_ENABLED(CLK)
>
> Why is the ifdef protecting only half of the clock functions ?
Because the clk_get_bulk() also has function define even if CONFIG_CLK is not defined.
So this way would not cause undefined reference error.
But for the sake of uniformity, I will move clk_get_bulk() into the ifdef protecting.
>
> > + ret = clk_enable_bulk(&priv->clks);
> > + if (ret) {
> > + clk_release_bulk(&priv->clks);
> > + return ret;
> > + }
> > +#endif
> > +
> > + return 0;
> > +}
>
>
> [...]
>
> > +static void cdns3_set_role(struct cdns3 *cdns, enum cdns3_roles role)
> > +{
> > + u32 value;
> > + int timeout_us = 100000;
> > + struct cdns3_generic_peripheral *priv = container_of(cdns,
> > + struct cdns3_generic_peripheral, cdns3);
> > +
> > + if (role == CDNS3_ROLE_END)
> > + return;
> > +
> > + /* Wait clk value */
> > + value = readl(cdns->none_core_regs + USB3_SSPHY_STATUS);
> > + writel(value, cdns->none_core_regs + USB3_SSPHY_STATUS);
> > + udelay(1);
> > + value = readl(cdns->none_core_regs + USB3_SSPHY_STATUS);
> > + while ((value & 0xf0000000) != 0xf0000000 && timeout_us-- > 0) {
> > + value = readl(cdns->none_core_regs + USB3_SSPHY_STATUS);
> > + udelay(1);
> > + }
>
> Is this like wait_for_bit() ?
Yes, thanks for the reminder, I have already use wait_for_bit_le32() to replace these.
>
> > + if (timeout_us <= 0)
> > + dev_err(cdns->dev, "wait clkvld timeout\n");
> > +
> > + /* Set all Reset bits */
> > + setbits_le32(cdns->none_core_regs + USB3_CORE_CTRL1,
> ALL_SW_RESET);
> > + udelay(1);
> > +
> > + if (role == CDNS3_ROLE_HOST) {
>
> Do we need custom controller role definition ? I don't think so, just use the
> one in the USB stack.
>
Actually we need create an array with two elements to store the corresponding host/gadge
function pointer in struct cdns3. And use CDNS3_ROLE_END to do the check.
If we use the role define in USB stack, the array[USB_DR_MODE_UNKNOWN] will always be
defined but not used. So I think the controller role definition is needed here.
But if you really think the controller role definition isn’t appropriate, I will change it.
10 enum cdns3_roles {
11 CDNS3_ROLE_HOST = 0,
12 CDNS3_ROLE_GADGET,
13 CDNS3_ROLE_END,
14 };
12 enum usb_dr_mode {
13 USB_DR_MODE_UNKNOWN,
14 USB_DR_MODE_HOST,
15 USB_DR_MODE_PERIPHERAL,
16 USB_DR_MODE_OTG,
17 };
> Also, use clrsetbits_le32()
Sure.
>
> > + value = readl(cdns->none_core_regs + USB3_CORE_CTRL1);
> > + value = (value & ~MODE_STRAP_MASK) | HOST_MODE |
> OC_DISABLE;
> > + writel(value, cdns->none_core_regs + USB3_CORE_CTRL1);
> > + clrbits_le32(cdns->none_core_regs + USB3_CORE_CTRL1,
> > + PHYAHB_SW_RESET);
> > + mdelay(1);
> > + generic_phy_init(&priv->phy);
> > + setbits_le32(cdns->phy_regs + TB_ADDR_TX_RCVDETSC_CTRL,
> > + RXDET_IN_P3_32KHZ);
> > + udelay(10);
> > + /* Force B Session Valid as 1 */
> > + writel(0x0060, cdns->phy_regs + 0x380a4);
> > + mdelay(1);
> > +
> > + setbits_le32(cdns->none_core_regs + USB3_INT_REG,
> HOST_INT1_EN);
> > +
> > + clrbits_le32(cdns->none_core_regs + USB3_CORE_CTRL1,
> > + ALL_SW_RESET);
> > +
> > + dev_dbg(cdns->dev, "wait xhci_power_on_ready\n");
> > +
> > + value = readl(cdns->none_core_regs + USB3_CORE_STATUS);
> > + timeout_us = 100000;
> > + while (!(value & HOST_POWER_ON_READY) && timeout_us-- > 0) {
> > + value = readl(cdns->none_core_regs + USB3_CORE_STATUS);
> > + udelay(1);
> > + }
> > +
> > + if (timeout_us <= 0)
> > + dev_err(cdns->dev, "wait xhci_power_on_ready timeout\n");
> > +
> > + dev_dbg(cdns->dev, "switch to host role successfully\n");
> > + } else { /* gadget mode */
>
> Split this into sensibly long funtions please.
Yes, I have done this.
>
> > + value = readl(cdns->none_core_regs + USB3_CORE_CTRL1);
> > + value = (value & ~MODE_STRAP_MASK) | DEV_MODE;
> > + writel(value, cdns->none_core_regs + USB3_CORE_CTRL1);
> > + clrbits_le32(cdns->none_core_regs + USB3_CORE_CTRL1,
> > + PHYAHB_SW_RESET);
> > +
> > + generic_phy_init(&priv->phy);
> > + setbits_le32(cdns->phy_regs + TB_ADDR_TX_RCVDETSC_CTRL,
> > + RXDET_IN_P3_32KHZ);
> > + udelay(10);
> > + /* Force B Session Valid as 1 */
> > + writel(0x0060, cdns->phy_regs + 0x380a4);
> > + setbits_le32(cdns->none_core_regs + USB3_INT_REG, DEV_INT_EN);
> > +
> > + clrbits_le32(cdns->none_core_regs + USB3_CORE_CTRL1,
> > + ALL_SW_RESET);
> > +
> > + dev_dbg(cdns->dev, "wait gadget_power_on_ready\n");
> > +
> > + value = readl(cdns->none_core_regs + USB3_CORE_STATUS);
> > + timeout_us = 100000;
> > + while (!(value & DEV_POWER_ON_READY) && timeout_us-- > 0) {
> > + value = readl(cdns->none_core_regs + USB3_CORE_STATUS);
> > + udelay(1);
> > + }
>
> wait_for_bit()
>
> [...]
>
> > +/* macros for field CFGSTS */
> > +#define USB_STS__CFGSTS__MASK
> 0x00000001U
> > +#define USB_STS__USBSPEED__READ(src) (((uint32_t)(src) &
> 0x00000070U) >> 4)
>
> u32, fix globally
Yes, I have fixed this.
>
> [...]
>
> > +static void __cdns3_gadget_start(struct usb_ss_dev *usb_ss) {
> > + u32 usb_conf_reg = 0;
> > +
> > + /* configure endpoint 0 hardware */
> > + cdns_ep0_config(usb_ss);
> > +
> > + /* enable interrupts for endpoint 0 (in and out) */
> > + cdns_writel(&usb_ss->regs->ep_ien,
> > + EP_IEN__EOUTEN0__MASK | EP_IEN__EINEN0__MASK);
> > +
> > + /* enable interrupt for device */
> > + cdns_writel(&usb_ss->regs->usb_ien,
> > + USB_IEN__U2RESIEN__MASK
> > + | USB_ISTS__DIS2I__MASK
> > + | USB_IEN__CON2IEN__MASK
> > + | USB_IEN__UHRESIEN__MASK
> > + | USB_IEN__UWRESIEN__MASK
> > + | USB_IEN__DISIEN__MASK
> > + | USB_IEN__CONIEN__MASK
> > + | USB_IEN__U3EXTIEN__MASK
> > + | USB_IEN__L2ENTIEN__MASK
> > + | USB_IEN__L2EXTIEN__MASK);
> > +
> > + usb_conf_reg = USB_CONF__CLK2OFFDS__MASK |
> > + USB_CONF__L1DS__MASK;
> > + if (usb_ss->gadget.max_speed == USB_SPEED_HIGH)
> > + usb_conf_reg |= USB_CONF__USB3DIS__MASK;
> > + cdns_writel(&usb_ss->regs->usb_conf, usb_conf_reg);
> > +
> > + cdns_writel(&usb_ss->regs->usb_conf,
> > + USB_CONF__U1DS__MASK
> > + | USB_CONF__U2DS__MASK
> > + /*
> > + * TODO:
> > + * | USB_CONF__L1EN__MASK
> > + */
> > + );
>
> Fix the TODO ?
I have already fixed this.
Best regards
Sherry sun
More information about the U-Boot
mailing list