Hi,
On Sat, Feb 09, 2013 at 07:37:48PM -0800, Paul Zimmerman wrote:
> +static void dwc2_core_reset(struct dwc2_hcd *hcd)
not sure if I would call this an HCD. Surely it can _be_ an HCD, but
it's actually a DRD device (not all configurations, but still).
If you call this HCD, it will be a little misleading when peripheral
mode is merged here IMO. How about:
sed -i 's/dwc2_hcd\s\*\(\w\+\)/dwc2 *dwc/g' ?
> +{
> + u32 greset;
> + int count = 0;
> +
> + dev_vdbg(hcd->dev, "%s()\n", __func__);
> +
> + /* Wait for AHB master IDLE state */
> + do {
> + msleep(20);
> + greset = readl(hcd->regs + GRSTCTL);
> + if (++count > 50) {
> + dev_warn(hcd->dev, "%s() HANG! AHB Idle GRSTCTL=%0x\n",
> + __func__, greset);
> + return;
> + }
> + } while (!(greset & GRSTCTL_AHBIDLE));
> +
> + /* Core Soft Reset */
> + count = 0;
> + greset |= GRSTCTL_CSFTRST;
> + writel(greset, hcd->regs + GRSTCTL);
> + do {
> + msleep(20);
> + greset = readl(hcd->regs + GRSTCTL);
> + if (++count > 50) {
> + dev_warn(hcd->dev,
> + "%s() HANG! Soft Reset GRSTCTL=%0x\n",
> + __func__, greset);
> + break;
> + }
> + } while (greset & GRSTCTL_CSFTRST);
> +
> + /*
> + * NOTE: This long sleep is _very_ important, otherwise the core will
> + * not stay in host mode after a connector ID change!
> + */
> + msleep(150);
I believe these three msleeps() don't really need that much accuracy,
right ? So how about converthem them to:
usleep_range(10000, 30000);
usleep_range(10000, 30000);
usleep_range(150000, 200000);
respectively ? It will give scheduler the change to group timers.
> +static void dwc2_phy_init(struct dwc2_hcd *hcd)
> +{
> + u32 usbcfg, i2cctl, hs_phy_type, fs_phy_type;
> +
> + if (hcd->core_params->speed == DWC2_SPEED_PARAM_FULL &&
> + hcd->core_params->phy_type == DWC2_PHY_TYPE_PARAM_FS) {
> + /* If FS mode with FS PHY */
> + /*
> + * core_init() is now called on every switch so only call the
> + * following for the first time through
> + */
> + if (!hcd->phy_init_done) {
> + hcd->phy_init_done = 1;
I wonder if this flag is really necessary. Seems like you can easily
make sure that this doesn't get called twice.
> + dev_dbg(hcd->dev, "FS_PHY detected\n");
nit-picking... phy isn't really "detected" here.
> + usbcfg = readl(hcd->regs + GUSBCFG);
> + usbcfg |= GUSBCFG_PHYSEL;
> + writel(usbcfg, hcd->regs + GUSBCFG);
> +
> + /* Reset after a PHY select */
> + dwc2_core_reset(hcd);
> + }
> +
> + /*
> + * Program DCFG.DevSpd or HCFG.FSLSPclkSel to 48Mhz in FS. Also
> + * do this on HNP Dev/Host mode switches (done in dev_init and
> + * host_init).
> + */
> + if (dwc2_is_host_mode(hcd))
> + dwc2_init_fs_ls_pclk_sel(hcd);
> +
> + if (hcd->core_params->i2c_enable > 0) {
seems like you could read this out of a register instead of using module
parameter, no ?
> + dev_dbg(hcd->dev, "FS_PHY Enabling I2c\n");
> +
> + /* Program GUSBCFG.OtgUtmiFsSel to I2C */
> + usbcfg = readl(hcd->regs + GUSBCFG);
> + usbcfg |= GUSBCFG_OTG_UTMI_FS_SEL;
> + writel(usbcfg, hcd->regs + GUSBCFG);
> +
> + /* Program GI2CCTL.I2CEn */
> + i2cctl = readl(hcd->regs + GI2CCTL);
> + i2cctl &= ~GI2CCTL_I2CDEVADDR_MASK;
> + i2cctl |= 1 << GI2CCTL_I2CDEVADDR_SHIFT;
> + i2cctl &= ~GI2CCTL_I2CEN;
> + writel(i2cctl, hcd->regs + GI2CCTL);
> + i2cctl |= GI2CCTL_I2CEN;
> + writel(i2cctl, hcd->regs + GI2CCTL);
btw, it looks like the two outter branches here could be factored out to
other functions and called like:
if (speed == FULL && phy_type == FULL)
dwc2_fs_phy_init(dwc);
else
dwc2_hs_phy_init(dwc);
> + }
> + } else {
> + /* High speed PHY */
> + if (!hcd->phy_init_done) {
> + hcd->phy_init_done = 1;
> + usbcfg = readl(hcd->regs + GUSBCFG);
> +
> + /*
> + * HS PHY parameters. These parameters are preserved
> + * during soft reset so only program the first time. Do
> + * a soft reset immediately after setting phyif.
> + */
> + if (hcd->core_params->phy_type == 2) {
few lines above you have a symbolic macro for phy_type, right here
you're using magic constants. Can you use symbolic macros here too ?
Also, I guess this would look a little nicer with a switch statement.
> + /* ULPI interface */
> + usbcfg |= GUSBCFG_ULPI_UTMI_SEL;
> + usbcfg &= ~(GUSBCFG_PHYIF16 | GUSBCFG_DDRSEL);
> + if (hcd->core_params->phy_ulpi_ddr > 0)
> + usbcfg |= GUSBCFG_DDRSEL;
> + } else if (hcd->core_params->phy_type == 1) {
> + /* UTMI+ interface */
> + usbcfg &= ~(GUSBCFG_ULPI_UTMI_SEL |
> + GUSBCFG_PHYIF16);
> + if (hcd->core_params->phy_utmi_width == 16)
> + usbcfg |= GUSBCFG_PHYIF16;
> + } else {
> + dev_err(hcd->dev, "FS PHY TYPE\n");
> + }
> +
> + writel(usbcfg, hcd->regs + GUSBCFG);
> +
> + /* Reset after setting the PHY parameters */
> + dwc2_core_reset(hcd);
since this is called on both branches, I guess you could factor it out
of the branches, no ?
> + }
> + }
> +
> + hs_phy_type = hcd->hwcfg2 & GHWCFG2_HS_PHY_TYPE_MASK;
> + fs_phy_type = hcd->hwcfg2 & GHWCFG2_FS_PHY_TYPE_MASK;
> +
> + if (hs_phy_type == GHWCFG2_HS_PHY_TYPE_ULPI &&
> + fs_phy_type == GHWCFG2_FS_PHY_TYPE_DEDICATED &&
> + hcd->core_params->ulpi_fs_ls > 0) {
> + dev_dbg(hcd->dev, "Setting ULPI FSLS\n");
> + usbcfg = readl(hcd->regs + GUSBCFG);
> + usbcfg |= GUSBCFG_ULPI_FS_LS;
> + usbcfg |= GUSBCFG_ULPI_CLK_SUSP_M;
> + writel(usbcfg, hcd->regs + GUSBCFG);
> + } else {
> + usbcfg = readl(hcd->regs + GUSBCFG);
> + usbcfg &= ~GUSBCFG_ULPI_FS_LS;
> + usbcfg &= ~GUSBCFG_ULPI_CLK_SUSP_M;
> + writel(usbcfg, hcd->regs + GUSBCFG);
> + }
> +}
> +
> +static int dwc2_gahbcfg_init(struct dwc2_hcd *hcd)
> +{
> + u32 ahbcfg = 0;
> + int i;
> +
> + switch (hcd->hwcfg2 & GHWCFG2_ARCHITECTURE_MASK) {
> + case GHWCFG2_EXT_DMA_ARCH:
> + dev_err(hcd->dev, "External DMA Mode not supported\n");
> + return -EINVAL;
> +
> + case GHWCFG2_INT_DMA_ARCH:
> + dev_dbg(hcd->dev, "Internal DMA Mode\n");
> + /*
> + * Old value was GAHBCFG_HBSTLEN_INCR - done for
> + * Host mode ISOC in issue fix - vahrama
> + */
> + ahbcfg |= GAHBCFG_HBSTLEN_INCR4;
> + break;
not really needed right now, but you should start considering dma engine
support.
> +static void dwc2_gusbcfg_init(struct dwc2_hcd *hcd)
> +{
> + u32 usbcfg;
> +
> + usbcfg = readl(hcd->regs + GUSBCFG);
> + usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP);
> +
> + switch (hcd->hwcfg2 & GHWCFG2_OP_MODE_MASK) {
> + case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE:
> + if (hcd->core_params->otg_cap == DWC2_CAP_PARAM_HNP_SRP_CAPABLE)
I'm assuming core_params is passed via module parameter, if that's the
case, then I'm not sure you need a module parameter. You could be using
DT/platform_data since this is board/platform-specific and won't change
in runtime.
If you expose to a module parameter, then a not-so-careful user might
start reporting bugs which don't exist just because he didn't know how
to pass the proper module parameter.
> +/**
> + * dwc2_core_init() - Initializes the DWC_otg controller registers and
> + * prepares the core for device mode or host mode operation
> + *
> + * @hcd: Programming view of the DWC_otg controller
> + */
> +int dwc2_core_init(struct dwc2_hcd *hcd)
is this still being called by the PCI layer ? On your cover letter you
said they're now two separate drivers, if that's the case, this should
be static and called on this driver's probe().
> + dev_dbg(hcd->dev, "Total FIFO SZ=%d\n", hcd->total_fifo_size);
> + dev_dbg(hcd->dev, "RxFIFO SZ=%d\n", hcd->rx_fifo_size);
> + dev_dbg(hcd->dev, "NP TxFIFO SZ=%d\n", hcd->nperio_tx_fifo_size);
these look like candidates for dev_vdbg() (so do many of the other
debugging messages around this driver :-)
> + /* Program the GLPMCFG register */
> + if (hcd->core_params->lpm_enable > 0) {
> + u32 lpmcfg = readl(hcd->regs + GLPMCFG);
> +
> + /* To enable LPM support set lpm_cap_en bit */
> + lpmcfg |= GLPMCFG_LPM_CAP_EN;
> +
> + /* Make AppL1Res ACK */
> + lpmcfg |= GLPMCFG_APPL_RESP;
> +
> + /* Retry 3 times */
> + lpmcfg &= ~GLPMCFG_RETRY_COUNT_MASK;
> + lpmcfg |= 3 << GLPMCFG_RETRY_COUNT_SHIFT;
> +
> + writel(lpmcfg, hcd->regs + GLPMCFG);
> + }
I would suggest to start without any LPM code at all. Any PM-related
features have the tendency to break things, so better to leave those out
until code is stable. You might be inclined to reply with 'but it's the
same thing as keeping the module parameter as zero' and, fair enough,
functionaly it might be; OTOH you're setting yourself up to have more
code to refactor later when you decide to clean some of the stuff up.
> + /* Program the GOTGCTL register */
> + otgctl = readl(hcd->regs + GOTGCTL);
> + otgctl &= ~GOTGCTL_OTGVER;
> + if (hcd->core_params->otg_ver > 0)
> + otgctl |= GOTGCTL_OTGVER;
> + writel(otgctl, hcd->regs + GOTGCTL);
> + dev_dbg(hcd->dev, "OTG VER PARAM: %d\n", hcd->core_params->otg_ver);
> +
> + /* Clear the SRP success bit for FS-I2c */
> + hcd->srp_success = 0;
wasn't 'hcd' allocated with kzalloc ? (btw, would be nice if you were
using devm_* API all over this driver, where possible).
> + /* Enable common interrupts */
> + dwc2_enable_common_interrupts(hcd);
before this call, there should be a request_irq() (preferrably a
devm_request_irq()).
> + /*
> + * Do device or host intialization based on mode during PCD and
> + * HCD initialization
> + */
> + if (dwc2_is_host_mode(hcd)) {
> + dev_dbg(hcd->dev, "Host Mode\n");
> + hcd->op_state = A_HOST;
we have generic implementations for this, look into <linux/usb/phy.h>
> +static void dwc2_config_fifos(struct dwc2_hcd *hcd)
> +{
> + struct dwc2_core_params *params = hcd->core_params;
> + u32 rxfsiz, nptxfsiz, ptxfsiz, hptxfsiz, dfifocfg;
> +
> + if ((hcd->hwcfg2 & GHWCFG2_DYNAMIC_FIFO) &&
> + params->enable_dynamic_fifo) {
you could decrease indentation here:
if (!(hcd->hwcfg2 & DYNAMIC && params->dynamic))
return;
> +static void dwc2_hc_enable_ints(struct dwc2_hcd *hcd, struct dwc2_hc *hc)
> +{
> + u8 hc_num = hc->hc_num;
> + u32 hc_intr_mask = HCINTMSK_CHHLTD;
> + u32 intr_enable, intmsk;
> +
> + if (hcd->core_params->dma_enable > 0) {
> + dev_vdbg(hcd->dev, "DMA enabled\n");
> + /*
> + * For Descriptor DMA mode core halts the channel on AHB error.
> + * Interrupt is not required.
> + */
> + if (hcd->core_params->dma_desc_enable <= 0) {
> + dev_vdbg(hcd->dev, "desc DMA disabled\n");
> + hc_intr_mask |= HCINTMSK_AHBERR;
> + } else {
> + dev_vdbg(hcd->dev, "desc DMA enabled\n");
> + if (hc->ep_type == USB_ENDPOINT_XFER_ISOC)
> + hc_intr_mask |= HCINTMSK_XFERCOMPL;
> + }
> +
> + if (hc->error_state && !hc->do_split &&
> + hc->ep_type != USB_ENDPOINT_XFER_ISOC) {
> + dev_vdbg(hcd->dev, "setting ACK\n");
> + hc_intr_mask |= HCINTMSK_ACK;
> + if (hc->ep_is_in) {
> + hc_intr_mask |= HCINTMSK_DATATGLERR;
> + if (hc->ep_type != USB_ENDPOINT_XFER_INT)
> + hc_intr_mask |= HCINTMSK_NAK;
> + }
> + }
> + } else {
this looks like another one of those cases where you could factor both
branches to separate functions.
> +void dwc2_hc_start_transfer(struct dwc2_hcd *hcd, struct dwc2_hc *hc)
dwc2_hc is a bit too short, IMO. I didn't know, at first, if HC meant
Host Controller or Host Channel (apparently it's Host Channel), so how
about:
sed -i 's/dwc2_hc *hc/dwc2_host_channel *channel/g' *
Also, this function seems to be dealing with PIO only, so how about
appending the name with _pio to make that clear ?
> +int dwc2_hc_continue_transfer(struct dwc2_hcd *hcd, struct dwc2_hc *hc)
> +{
> + dev_vdbg(hcd->dev, "%s: Channel %d\n", __func__, hc->hc_num);
> +
> + if (hc->do_split) {
> + /* SPLITs always queue just once per channel */
> + return 0;
> + } else if (hc->data_pid_start == DWC2_HC_PID_SETUP) {
> + /* SETUPs are queued only once since they can't be NAK'd */
> + return 0;
> + } else if (hc->ep_is_in) {
> + /*
> + * Always queue another request for other IN transfers. If
> + * back-to-back INs are issued and NAKs are received for both,
> + * the driver may still be processing the first NAK when the
> + * second NAK is received. When the interrupt handler clears
> + * the NAK interrupt for the first NAK, the second NAK will
> + * not be seen. So we can't depend on the NAK interrupt
> + * handler to requeue a NAK'd request. Instead, IN requests
> + * are issued each time this function is called. When the
> + * transfer completes, the extra requests for the channel will
> + * be flushed.
> + */
> + u32 hcchar = readl(hcd->regs + HCCHAR(hc->hc_num));
> +
> + dwc2_hc_set_even_odd_frame(hcd, hc, &hcchar);
> + hcchar |= HCCHAR_CHENA;
> + hcchar &= ~HCCHAR_CHDIS;
> + dev_vdbg(hcd->dev, " IN xfer: hcchar = 0x%08x\n", hcchar);
> + writel(hcchar, hcd->regs + HCCHAR(hc->hc_num));
> + hc->requests++;
> + return 1;
> + } else {
> + /* OUT transfers */
> + if (hc->xfer_count < hc->xfer_len) {
> + if (hc->ep_type == USB_ENDPOINT_XFER_INT ||
> + hc->ep_type == USB_ENDPOINT_XFER_ISOC) {
> + u32 hcchar = readl(hcd->regs +
> + HCCHAR(hc->hc_num));
> +
> + dwc2_hc_set_even_odd_frame(hcd, hc, &hcchar);
> + }
> +
> + /* Load OUT packet into the appropriate Tx FIFO */
> + dwc2_hc_write_packet(hcd, hc);
> + hc->requests++;
> + return 1;
> + } else {
> + return 0;
> + }
> + }
doesn't look like you need the else statements anywhere here, since you
always return from each branch. You might as well:
if (hc->do_split) {
foo();
return 0;
}
if (hc->data_pid_start == SETUP) {
bar();
return 0;
}
if (hc->ep_is_in) {
baz();
return 1;
}
if (hc->xfer_count < hc->xfer_len) {
fooish_bar();
return 1;
} else {
return 0;
}
> +void dwc2_dump_host_registers(struct dwc2_hcd *hcd)
you should probably mention on this function's documentation that it's
scheduled for removal after driver is more stable, so people don't
really depend on it.
> +void dwc2_dump_global_registers(struct dwc2_hcd *hcd)
ditto
> +void dwc2_flush_tx_fifo(struct dwc2_hcd *hcd, const int num)
> +{
> + u32 greset;
> + int count = 0;
> +
> + dev_vdbg(hcd->dev, "Flush Tx FIFO %d\n", num);
> +
> + greset = GRSTCTL_TXFFLSH;
> + greset |= num << GRSTCTL_TXFNUM_SHIFT & GRSTCTL_TXFNUM_MASK;
> + writel(greset, hcd->regs + GRSTCTL);
> +
> + do {
> + greset = readl(hcd->regs + GRSTCTL);
> + if (++count > 10000) {
> + dev_warn(hcd->dev,
> + "%s() HANG! GRSTCTL=%0x GNPTXSTS=0x%08x\n",
> + __func__, greset, readl(hcd->regs + GNPTXSTS));
> + break;
> + }
> + udelay(1);
> + } while (greset & GRSTCTL_TXFFLSH);
> +
> + /* Wait for at least 3 PHY Clocks */
> + udelay(1);
could you use usleep_range() here ?
> +void dwc2_flush_rx_fifo(struct dwc2_hcd *hcd)
> +{
> + u32 greset;
> + int count = 0;
> +
> + dev_vdbg(hcd->dev, "%s()\n", __func__);
> +
> + greset = GRSTCTL_RXFFLSH;
> + writel(greset, hcd->regs + GRSTCTL);
> +
> + do {
> + greset = readl(hcd->regs + GRSTCTL);
> + if (++count > 10000) {
> + dev_warn(hcd->dev, "%s() HANG! GRSTCTL=%0x\n",
> + __func__, greset);
> + break;
> + }
> + udelay(1);
> + } while (greset & GRSTCTL_RXFFLSH);
> +
> + /* Wait for at least 3 PHY Clocks */
> + udelay(1);
and here ?
> +void dwc2_enable_global_interrupts(struct dwc2_hcd *hcd)
> +{
> + u32 ahbcfg = readl(hcd->regs + GAHBCFG);
> +
> + ahbcfg |= GAHBCFG_GLBL_INTR_EN;
> + writel(ahbcfg, hcd->regs + GAHBCFG);
> +}
> +EXPORT_SYMBOL(dwc2_enable_global_interrupts);
> +
> +/**
> + * dwc2_disable_global_interrupts() - Disables the controller's Global
> + * Interrupt in the AHB Config register
> + *
> + * @hcd: Programming view of DWC_otg controller
> + */
> +void dwc2_disable_global_interrupts(struct dwc2_hcd *hcd)
> +{
> + u32 ahbcfg = readl(hcd->regs + GAHBCFG);
> +
> + ahbcfg &= ~GAHBCFG_GLBL_INTR_EN;
> + writel(ahbcfg, hcd->regs + GAHBCFG);
> +}
> +EXPORT_SYMBOL(dwc2_disable_global_interrupts);
> +
> +u32 dwc2_get_gsnpsid(void __iomem *base)
> +{
> + void __iomem *addr = (void __iomem *)(base + GSNPSID);
> +
> + return readl(addr);
> +}
> +EXPORT_SYMBOL(dwc2_get_gsnpsid);
Frankly, I'm still not a fan of the amount of exposed functions you have
on this driver, but this is your headache, so I won't bother anymore. I
have already tried to explain why I think following what we did on dwc3
pays off in the long run...
Anyway, what I want to say here is:
can you at least use EXPORT_SYMBOL_GPL(), or does that taint this same
driver because it's Dual BSD/GPL ?
> +/* Macros defined for DWC OTG HW Release version */
> +#define DWC2_CORE_REV_2_60a 0x4f54260a
> +#define DWC2_CORE_REV_2_71a 0x4f54271a
> +#define DWC2_CORE_REV_2_72a 0x4f54272a
> +#define DWC2_CORE_REV_2_80a 0x4f54280a
> +#define DWC2_CORE_REV_2_81a 0x4f54281a
> +#define DWC2_CORE_REV_2_90a 0x4f54290a
> +#define DWC2_CORE_REV_2_91a 0x4f54291a
> +#define DWC2_CORE_REV_2_92a 0x4f54292a
> +#define DWC2_CORE_REV_2_93a 0x4f54293a
> +#define DWC2_CORE_REV_2_94a 0x4f54294a
> +#define DWC2_CORE_REV_3_00a 0x4f54300a
I guess it would be best to define these closer to where they're used.
Note that we have defined them inside struct dwc3 on the other driver.
It makes it easier for the silly human reading the code to note that
these are the supported revisions.
> +extern void dwc2_core_host_init(struct dwc2_hcd *hcd);
hehe, I couldn't resist:
$ grep -c "extern \([a-z0-9]\+\) [a-z0-9]\+" patch.diff
52
> +static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hcd *hcd)
> +{
> + u32 gintmsk = readl(hcd->regs + GINTMSK);
> +
> + /*
> + * Need to disable SOF interrupt immediately. If switching from device
> + * to host, the PCD interrupt handler won't handle the interrupt if
> + * host mode is already set. The HCD interrupt handler won't get
> + * called if the HCD state is HALT. This means that the interrupt does
> + * not get handled and Linux complains loudly.
> + */
> + gintmsk &= ~GINTSTS_SOF;
> + writel(gintmsk, hcd->regs + GINTMSK);
> +
> + dev_dbg(hcd->dev, " ++Connector ID Status Change Interrupt++ (%s)\n",
> + dwc2_is_host_mode(hcd) ? "Host" : "Device");
> +
> + /*
> + * Need to schedule a work, as there are possible DELAY function calls.
> + * Release lock before scheduling workq as it holds spinlock during
> + * scheduling.
> + */
> + spin_unlock(&hcd->lock);
> + queue_work(hcd->wq_otg, &hcd->wf_otg);
please don't, convert to devm_request_threaded_irq() and run your IRQ
handler on a thread.
Ideally, you should use hardirq solely to check if this device generated
the IRQ (yes, I know dwc3 isn't doing that, I have plans to fix it but
have other more important stuff to do first ;-)
> +static void dwc2_handle_session_req_intr(struct dwc2_hcd *hcd)
> +{
> + dev_dbg(hcd->dev, "++Session Request Interrupt++\n");
> +
> +#ifndef DWC2_HOST_ONLY
> + if (dwc2_is_device_mode(hcd)) {
> + dev_info(hcd->dev, "SRP: Device mode\n");
> + } else {
> + u32 hprt0;
> +
> + dev_info(hcd->dev, "SRP: Host mode\n");
> +
> + /* Turn on the port power bit */
> + hprt0 = dwc2_read_hprt0(hcd);
> + hprt0 |= HPRT0_PWR;
> + writel(hprt0, hcd->regs + HPRT0);
> +
> + /*
> + * Start the Connection timer. So a message can be displayed if
> + * connect does not occur within 10 seconds.
> + */
> + dwc2_hcd_session_start(hcd);
> + }
> +#endif
I think you should drop the ifdef here. Any host is allowed to support
SRP, not only embedded hosts or OTG devices.
(one down, 4 more to go).
--
balbi
Attachment:
signature.asc
Description: Digital signature
