Re: [PATCH 1/4] [SCSI] ufshcd: UFS Host controller driver

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 7346 bytes --]

Hi,

On Thu, Feb 02, 2012 at 10:27:26AM +0530, Vinayak Holikatti wrote:
> +/**
> + * ufshcd_uic_cc_handler - handle UIC command completion
> + * @work: pointer to a work queue structure
> + *
> + * Returns 0 on success, non-zero value on failure
> + */
> +static void ufshcd_uic_cc_handler (struct work_struct *work)
> +{
> +	struct ufs_hba *hba;
> +
> +	hba = container_of(work, struct ufs_hba, uic_workq);
> +
> +	if ((UIC_CMD_DME_LINK_STARTUP == hba->active_uic_cmd.command) &&

this is so annoying. Please invert all these constructs:

if ((hcd->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) ....
and so on.

> +	    !(ufshcd_get_uic_cmd_result(hba))) {
> +
> +		if (ufshcd_make_hba_operational(hba))
> +			dev_err(&hba->pdev->dev,
> +				"cc: hba not operational state\n");
> +		return;
> +	}
> +}
> +
> +/**
> + * ufshcd_sl_intr - Interrupt service routine
> + * @hba: per adapter instance
> + * @intr_status: contains interrupts generated by the controller
> + */
> +static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> +{
> +	if (intr_status & UIC_COMMAND_COMPL)
> +		schedule_work(&hba->uic_workq);
> +}
> +
> +/**
> + * ufshcd_intr - Main interrupt service routine
> + * @irq: irq number
> + * @__hba: pointer to adapter instance
> + *
> + * Returns IRQ_HANDLED - If interrupt is valid
> + *		IRQ_NONE - If invalid interrupt
> + */
> +static irqreturn_t ufshcd_intr(int irq, void *__hba)
> +{
> +	unsigned long flags;
> +	u32 intr_status;
> +	irqreturn_t retval = IRQ_NONE;
> +	struct ufs_hba *hba = __hba;
> +
> +	spin_lock_irqsave(hba->host->host_lock, flags);
> +	intr_status = readl(UFSHCD_MMIO_BASE + REG_INTERRUPT_STATUS);
> +
> +	if (intr_status) {
> +		ufshcd_sl_intr(hba, intr_status);
> +
> +		/* If UFSHCI 1.0 then clear interrupt status register */
> +		if (UFSHCI_VERSION_10 == hba->ufs_version)
> +			writel(intr_status,
> +			       (UFSHCD_MMIO_BASE + REG_INTERRUPT_STATUS));
> +		retval = IRQ_HANDLED;
> +	}
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	return retval;
> +}
> +
> +static struct scsi_host_template ufshcd_driver_template = {
> +	.module			= THIS_MODULE,
> +	.name			= UFSHCD,
> +	.proc_name		= UFSHCD,
> +	.this_id		= -1,
> +};
> +
> +/**
> + * ufshcd_shutdown - main funciton to put the controller in reset state
> + * @pdev: pointer to PCI device handle
> + */
> +static void ufshcd_shutdown(struct pci_dev *pdev)
> +{
> +	ufshcd_hba_stop((struct ufs_hba *)pci_get_drvdata(pdev));
> +}
> +
> +#ifdef CONFIG_PM
> +/**
> + * ufshcd_suspend - suspend power management function
> + * @pdev: pointer to PCI device handle
> + * @state: power state
> + *
> + * Returns -ENOSYS
> + */
> +static int ufshcd_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	return -ENOSYS;
> +}
> +
> +/**
> + * ufshcd_resume - resume power management function
> + * @pdev: pointer to PCI device handle
> + *
> + * Returns -ENOSYS
> + */
> +static int ufshcd_resume(struct pci_dev *pdev)
> +{
> +	return -ENOSYS;
> +}
> +#endif /* CONFIG_PM */
> +
> +/**
> + * ufshcd_hba_free - free allocated memory for
> + *			host memory space data structures
> + * @hba: per adapter instance
> + */
> +static void ufshcd_hba_free(struct ufs_hba *hba)
> +{
> +	iounmap(UFSHCD_MMIO_BASE);
> +	ufshcd_free_hba_memory(hba);
> +	pci_release_regions(hba->pdev);
> +}
> +
> +/**
> + * ufshcd_remove - deallocate PCI/SCSI host and host memory space
> + *		data structure memory
> + * @pdev - pointer to PCI handle
> + */
> +static void ufshcd_remove(struct pci_dev *pdev)
> +{
> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
> +
> +	/* disable interrupts */
> +	ufshcd_int_config(hba, UFSHCD_INT_DISABLE);
> +	free_irq(pdev->irq, hba);
> +
> +	ufshcd_hba_stop(hba);
> +	ufshcd_hba_free(hba);
> +
> +	scsi_remove_host(hba->host);
> +	scsi_host_put(hba->host);
> +	pci_set_drvdata(pdev, NULL);
> +	pci_clear_master(pdev);
> +	pci_disable_device(pdev);
> +}
> +
> +/**
> + * ufshcd_set_dma_mask - Set dma addressing
> + * @pdev: PCI device struct
> + *
> + * Returns 0 for success, non-zero for failure
> + */
> +static int ufshcd_set_dma_mask(struct pci_dev *pdev)
> +{
> +	int err;
> +
> +	do {
> +		err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> +		if (!err) {
> +			err = pci_set_consistent_dma_mask(pdev,
> +							  DMA_BIT_MASK(64));
> +			break;
> +		}
> +		err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> +		if (!err)
> +			err = pci_set_consistent_dma_mask(pdev,
> +							  DMA_BIT_MASK(32));
> +	} while (0);
> +
> +	return err;
> +}
> +
> +/**
> + * ufshcd_probe - probe routine of the driver
> + * @pdev: pointer to PCI device handle
> + * @id: PCI device id
> + *
> + * Returns 0 on success, non-zero value on failure
> + */
> +static int __devinit
> +ufshcd_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct Scsi_Host *host;
> +	struct ufs_hba *hba;
> +	int ufs_hba_len;
> +	int err;
> +
> +	ufs_hba_len = sizeof(struct ufs_hba);
> +	err = pci_enable_device(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "pci_enable_device failed\n");
> +		goto out_error;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	host = scsi_host_alloc(&ufshcd_driver_template, ufs_hba_len);
> +	if (!host) {
> +		dev_err(&pdev->dev, "scsi_host_alloc failed\n");
> +		err = -ENOMEM;
> +		goto out_disable;
> +	}
> +	hba = (struct ufs_hba *)host->hostdata;
> +
> +	err = pci_request_regions(pdev, UFSHCD);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "request regions failed\n");
> +		goto out_disable;
> +	}
> +
> +	hba->mmio_base = pci_ioremap_bar(pdev, 0);
> +	if (!hba->mmio_base) {
> +		dev_err(&pdev->dev, "memory map failed\n");
> +		err = -ENOMEM;
> +		goto out_release_regions;
> +	}
> +
> +	err = ufshcd_set_dma_mask(pdev);
> +	if (err) {
> +		dev_err(&pdev->dev, "set dma mask failed\n");
> +		goto out_iounmap;
> +	}
> +
> +	hba->host = host;
> +	hba->pdev = pdev;
> +
> +	/* Read capabilities registers */
> +	ufshcd_hba_capabilities(hba);
> +
> +	/* Get UFS version supported by the controller */
> +	hba->ufs_version = ufshcd_get_ufs_version(hba);
> +
> +	/* Allocate memory for host memory space */
> +	err = ufshcd_memory_alloc(hba);
> +	if (err) {
> +		dev_err(&pdev->dev, "Memory allocation failed\n");
> +		goto out_iounmap;
> +	}
> +
> +	/* Configure LRB */
> +	ufshcd_host_memory_configure(hba);
> +
> +	host->can_queue = hba->nutrs;
> +	host->max_id = UFSHCD_MAX_ID;
> +	host->max_lun = UFSHCD_MAX_LUNS;
> +	host->max_channel = UFSHCD_MAX_CHANNEL;
> +	host->unique_id = host->host_no;
> +
> +	/* Initialize work queues */
> +	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
> +
> +	/* IRQ registration */
> +	err = request_irq(pdev->irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);

make this a threaded IRQ and remove your workqueues and tasklets.

> +	if (err) {
> +		dev_err(&pdev->dev, "request irq failed\n");
> +		goto out_lrb_free;
> +	}
> +
> +	pci_set_drvdata(pdev, hba);

would also be cool if you would abstract PCI out of this driver, so it's
easier to be re-used. See how I did it with drivers/usb/dwc3/core.c
drivers/usb/dwc3/dwc3-pci.c and drivers/usb/dwc3/dwc3-omap.c

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]