Re: [PATCH v4 4/5] PCI bus interface for the DWC2 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Tue, Feb 19, 2013 at 06:50:07PM -0800, Paul Zimmerman wrote:
> This file contains the PCI bus interface "glue" for the DWC2
> driver
> 
> Signed-off-by: Paul Zimmerman <paulz@xxxxxxxxxxxx>

before anything, out of curiosity, do you have DWC2 PCI configured with
OTG support ? I remember that on HAPS6x configuring DWC3 PCI OTG would
take too much space on the FPGA and that would cause difficulties on
timing closure, but perhaps dwc2 is smaller ?!?

> diff --git a/drivers/usb/dwc2/pci.c b/drivers/usb/dwc2/pci.c
> new file mode 100644
> index 0000000..899709f
> --- /dev/null
> +++ b/drivers/usb/dwc2/pci.c
> @@ -0,0 +1,355 @@
> +/*
> + * pci.c - DesignWare HS OTG Controller PCI driver
> + *
> + * Copyright (C) 2004-2013 Synopsys, Inc.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions, and the following disclaimer,
> + *    without modification.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. The names of the above-listed copyright holders may not be used
> + *    to endorse or promote products derived from this software without
> + *    specific prior written permission.
> + *
> + * ALTERNATIVELY, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") as published by the Free Software
> + * Foundation; either version 2 of the License, or (at your option) any
> + * later version.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> + * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +/*
> + * Provides the initialization and cleanup entry points for the DWC_otg PCI
> + * driver
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/spinlock.h>
> +#include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/debugfs.h>

not used

> +#include <linux/seq_file.h>

not used

> +#include <linux/delay.h>

aparently not used

> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/pci.h>
> +#include <linux/usb.h>
> +
> +#include <linux/usb/hcd.h>
> +#include <linux/usb/ch11.h>
> +#include <linux/usb/gadget.h>

gadget ? This is still host only, right ?

> +static void dwc2_driver_remove(struct pci_dev *dev)
> +{
> +	struct dwc2_device *otg_dev = pci_get_drvdata(dev);
> +
> +	dev_dbg(&dev->dev, "%s(%p)\n", __func__, dev);
> +
> +	if (!otg_dev) {
> +		dev_dbg(&dev->dev, "%s: otg_dev NULL!\n", __func__);
> +		return;
> +	}

this check can be removed, drivers core guarantees that pci_dev *dev
will always be true and, since you clearly and unconditionally call
pci_set_drvdata(), otg_dev will always be valid.

> +
> +	dev_dbg(&dev->dev, "otg_dev->hsotg = %p\n", otg_dev->hsotg);
> +	if (otg_dev->hsotg)
> +		dwc2_hcd_remove(&dev->dev, otg_dev);
> +	else
> +		dev_dbg(&dev->dev, "%s: otg_dev->hsotg NULL!\n", __func__);

this check can be removed I guess. You shouldn't be able to remove what
wasn't registered.

> +	/* Clear the drvdata pointer */
> +	pci_set_drvdata(dev, NULL);

this can be removed too, the driver is currently unbinding from the pci
device, so drvdata won't be accessed by anyone until next probe() which
will call pci_set_drvdata() to a different pointer anyway.

In summary, I believe this can be shortened to:

struct dwc3_device *otg_dev = pci_get_drvdata(dev);

dwc2_hcd_remove(&dev->dev, otg_dev);

return 0;

> +static int dwc2_driver_probe(struct pci_dev *dev,
> +			     const struct pci_device_id *id)
> +{
> +	struct dwc2_device *otg_dev;
> +	struct resource *res;
> +	int retval;
> +
> +	dev_dbg(&dev->dev, "%s(%p)\n", __func__, dev);
> +	dev_dbg(&dev->dev, "start=0x%08x\n",
> +		(unsigned)pci_resource_start(dev, 0));
> +
> +	otg_dev = devm_kzalloc(&dev->dev, sizeof(*otg_dev), GFP_KERNEL);
> +	if (!otg_dev)
> +		return -ENOMEM;
> +
> +	/* Map the DWC_otg Core memory into virtual address space */
> +	dev->current_state = PCI_D0;
> +	dev->dev.power.power_state = PMSG_ON;

no no no... you shouldn't access these directly.

Plese use pci_set_state(dev, PCI_D0) and
dev_pm_qos_constraints_init(&dev->dev);

> +	if (!dev->irq) {
> +		dev_err(&dev->dev,
> +			"Found HC with no IRQ. Check BIOS/PCI %s setup!",
> +			pci_name(dev));
> +		return -ENODEV;
> +	}

I don't think this can happen, unless your PCI address space is broken.

> +	otg_dev->rsrc_start = pci_resource_start(dev, 0);
> +	otg_dev->rsrc_len = pci_resource_len(dev, 0);
> +	dev_dbg(&dev->dev, "PCI resource: start=%08x, len=%08x\n",
> +		(unsigned)otg_dev->rsrc_start, (unsigned)otg_dev->rsrc_len);
> +
> +	res = devm_request_mem_region(&dev->dev, otg_dev->rsrc_start,
> +				      otg_dev->rsrc_len, dev_name(&dev->dev));

sidenote: it would be if there was a devm_pci_ioremap_bar() helper :-(

> +	if (!res)
> +		return -EBUSY;
> +
> +	otg_dev->base = devm_ioremap_nocache(&dev->dev, res->start,
> +					     resource_size(res));
> +	if (!otg_dev->base)
> +		return -ENOMEM;
> +
> +	dev_dbg(&dev->dev, "base=0x%p\n", otg_dev->base);
> +	dev_dbg(&dev->dev, "%s: mapped PA %1x to VA %p\n", __func__,
> +		(unsigned)otg_dev->rsrc_start, otg_dev->base);
> +
> +	if (pci_enable_device(dev) < 0) {
> +		dev_err(&dev->dev, "Invalid pci_device %p", dev);
> +		retval = -ENODEV;
> +		goto fail;
> +	}
> +
> +	pci_set_master(dev);
> +
> +	/*
> +	 * Initialize driver data to point to the global DWC_otg Device
> +	 * structure
> +	 */
> +	pci_set_drvdata(dev, otg_dev);
> +	dev_dbg(&dev->dev, "otg_dev=0x%p\n", otg_dev);
> +
> +	/* Set device flags indicating whether the HCD supports DMA */
> +	if (dwc2_module_params.dma_enable > 0) {
> +		pci_set_dma_mask(dev, DMA_BIT_MASK(32));
> +		pci_set_consistent_dma_mask(dev, DMA_BIT_MASK(32));
> +	} else {
> +		pci_set_dma_mask(dev, 0);
> +		pci_set_consistent_dma_mask(dev, 0);
> +	}
> +
> +	/* Initialize the HCD */
> +	retval = dwc2_hcd_init(&dev->dev, otg_dev, dev->irq,
> +			       &dwc2_module_params);
> +	/*
> +	 * WARNING: dwc2_hcd_init() calls usb_create_hcd(), which overwrites
> +	 * the pci_dev drvdata, so we must set it again here
> +	 */
> +	pci_set_drvdata(dev, otg_dev);

pci_set_drvdata() is called twice.

> +	if (retval != 0) {
> +		dev_err(&dev->dev, "dwc2_hcd_init failed\n");
> +		otg_dev->hsotg = NULL;
> +		goto fail;
> +	}
> +
> +	/* Install the interrupt handler for the common interrupts */
> +	dev_dbg(&dev->dev, "registering (common) handler for irq%d\n",
> +		dev->irq);
> +	retval = devm_request_irq(&dev->dev, dev->irq, dwc2_handle_common_intr,
> +				  IRQF_SHARED | IRQ_LEVEL, dev_name(&dev->dev),
> +				  otg_dev);
> +	if (retval) {
> +		retval = -EBUSY;
> +		goto fail;
> +	}
> +
> +	return 0;
> +
> +fail:
> +	dev_err(&dev->dev, "%s() FAILED, returning %d\n", __func__, retval);
> +	dwc2_driver_remove(dev);
> +	return retval;
> +}
> +
> +static DEFINE_PCI_DEVICE_TABLE(dwc2_pci_ids) = {
> +	{
> +		PCI_DEVICE(0x16c3, 0xabc0),

please add macros for the vendor ID and product ID, I think you want
something like:

#define PCI_VENDOR_ID_SYNOPSYS	0x16c3
#define PCI_PRODUCT_ID_HAPS	0xabc0

or something similar.

> +	},
> +	{ /* end: all zeroes */ }
> +};
> +MODULE_DEVICE_TABLE(pci, dwc2_pci_ids);
> +
> +static struct pci_driver dwc2_pci_driver = {
> +	.name = dwc2_driver_name,
> +	.id_table = dwc2_pci_ids,
> +	.probe = dwc2_driver_probe,
> +	.remove = dwc2_driver_remove,
> +};
> +
> +module_pci_driver(dwc2_pci_driver);
> +
> +module_param_named(otg_cap, dwc2_module_params.otg_cap, int, 0444);
> +MODULE_PARM_DESC(otg_cap, "OTG Capabilities 0=HNP+SRP 1=SRP-only 2=None");
> +
> +module_param_named(dma_enable, dwc2_module_params.dma_enable, int, 0444);
> +MODULE_PARM_DESC(dma_enable, "DMA Mode 0=Disabled 1=Enabled");
> +
> +module_param_named(dma_desc_enable, dwc2_module_params.dma_desc_enable, int,
> +		   0444);
> +MODULE_PARM_DESC(dma_desc_enable, "Descriptor DMA Mode 0=Disabled 1=Enabled");
> +
> +module_param_named(speed, dwc2_module_params.speed, int, 0444);
> +MODULE_PARM_DESC(speed, "Speed 0=High 1=Full");
> +
> +module_param_named(host_support_fs_ls_low_power,
> +		   dwc2_module_params.host_support_fs_ls_low_power, int,
> +		   0444);
> +MODULE_PARM_DESC(host_support_fs_ls_low_power,
> +		 "Support for Low Power w/FS or LS 0=Disabled 1=Enabled");
> +
> +module_param_named(host_ls_low_power_phy_clk,
> +		   dwc2_module_params.host_ls_low_power_phy_clk, int, 0444);
> +MODULE_PARM_DESC(host_ls_low_power_phy_clk,
> +		 "Low Speed Low Power Clock 0=48Mhz 1=6Mhz");
> +
> +module_param_named(enable_dynamic_fifo,
> +		   dwc2_module_params.enable_dynamic_fifo, int, 0444);
> +MODULE_PARM_DESC(enable_dynamic_fifo,
> +		 "Dynamic FIFO Sizing 0=Disabled 1=Enabled");
> +
> +module_param_named(host_rx_fifo_size, dwc2_module_params.host_rx_fifo_size,
> +		   int, 0444);
> +MODULE_PARM_DESC(host_rx_fifo_size, "Number of words in Rx FIFO 16-32768");
> +
> +module_param_named(host_nperio_tx_fifo_size,
> +		   dwc2_module_params.host_nperio_tx_fifo_size, int, 0444);
> +MODULE_PARM_DESC(host_nperio_tx_fifo_size,
> +		 "Number of words in non-periodic Tx FIFO 16-32768");
> +
> +module_param_named(host_perio_tx_fifo_size,
> +		   dwc2_module_params.host_perio_tx_fifo_size, int, 0444);
> +MODULE_PARM_DESC(host_perio_tx_fifo_size,
> +		 "Number of words in host periodic Tx FIFO 16-32768");
> +
> +module_param_named(max_transfer_size, dwc2_module_params.max_transfer_size,
> +		   int, 0444);
> +/* Todo: Set the max to 512K, modify checks */
> +MODULE_PARM_DESC(max_transfer_size,
> +		 "Maximum transfer size supported in bytes 2047-65535");
> +
> +module_param_named(max_packet_count, dwc2_module_params.max_packet_count,
> +		   int, 0444);
> +MODULE_PARM_DESC(max_packet_count,
> +		 "Maximum number of packets in a transfer 15-511");
> +
> +module_param_named(host_channels, dwc2_module_params.host_channels, int,
> +		   0444);
> +MODULE_PARM_DESC(host_channels, "Number of host channels to use 1-16");
> +
> +module_param_named(phy_type, dwc2_module_params.phy_type, int, 0444);
> +MODULE_PARM_DESC(phy_type, "Phy Type 0=Reserved 1=UTMI+ 2=ULPI");
> +
> +module_param_named(phy_utmi_width, dwc2_module_params.phy_utmi_width, int,
> +		   0444);
> +MODULE_PARM_DESC(phy_utmi_width, "Specifies the UTMI+ Data Width 8 or 16 bits");
> +
> +module_param_named(phy_ulpi_ddr, dwc2_module_params.phy_ulpi_ddr, int, 0444);
> +MODULE_PARM_DESC(phy_ulpi_ddr,
> +		 "ULPI at double or single data rate 0=Single 1=Double");
> +
> +module_param_named(phy_ulpi_ext_vbus, dwc2_module_params.phy_ulpi_ext_vbus,
> +		   int, 0444);
> +MODULE_PARM_DESC(phy_ulpi_ext_vbus,
> +	"ULPI PHY using internal or external VBus 0=Internal 1=External");
> +
> +module_param_named(i2c_enable, dwc2_module_params.i2c_enable, int, 0444);
> +MODULE_PARM_DESC(i2c_enable, "FS PHY Interface (I2C) 0=No 1=Yes");
> +
> +module_param_named(ulpi_fs_ls, dwc2_module_params.ulpi_fs_ls, int, 0444);
> +MODULE_PARM_DESC(ulpi_fs_ls, "ULPI PHY FS/LS mode only 0=No 1=Yes");
> +
> +module_param_named(ts_dline, dwc2_module_params.ts_dline, int, 0444);
> +MODULE_PARM_DESC(ts_dline, "Term select Dline pulsing for all PHYs 0=No 1=Yes");
> +
> +module_param_named(en_multiple_tx_fifo,
> +		   dwc2_module_params.en_multiple_tx_fifo, int, 0444);
> +MODULE_PARM_DESC(en_multiple_tx_fifo,
> +		 "Dedicated Non Periodic Tx FIFOs 0=Disabled 1=Enabled");
> +
> +module_param_named(reload_ctl, dwc2_module_params.reload_ctl, int, 0444);
> +MODULE_PARM_DESC(reload_ctl, "HFIR Reload Control 0=Disabled 1=Enabled");
> +
> +module_param_named(ahb_single, dwc2_module_params.ahb_single, int, 0444);
> +MODULE_PARM_DESC(ahb_single, "AHB Single Support 0=Disabled 1=Enabled");
> +
> +module_param_named(otg_ver, dwc2_module_params.otg_ver, int, 0444);
> +MODULE_PARM_DESC(otg_ver, "OTG revision supported 0=1.3 1=2.0");

My last comment about module parameters, you should be really careful
with the as they are valid for all boards available in the system, I
mean if you want to play with multiple dwc2 PCI cards on the same PC,
whatever module you pass for one card will be valid for the other too.

If someone decides to build a real PCIe silicon out of this RTL, things
could go a bit fishy.

-- 
balbi

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux